[cfe-dev] Proposed minor refactoring of the diag subsystem

Chris Lattner clattner at apple.com
Sat Aug 9 17:44:42 PDT 2008

On Aug 9, 2008, at 4:56 PM, Nico Weber wrote:
> Hi Chris,
>> I like some parts of this patch, but others are more questionable.   
>> The nice thing about libbasic right now is that it is C- 
>> indepedent.  The components in it are reusable for other languages,  
>> e.g. if you did a Java or Ada frontend.  HeaderSearch is quite C  
>> specific, so I'm somewhat uncomfortable with moving it into basic.
> I don't _quite_ buy that, as SourceLocation can contain a MacroID,  
> FileIDInfo has an IncludeLoc, etc -- there's C-dependence in Basic  
> everywhere ;-)

Fair enough, you caught my lie/exageration ;-)

>>> * Remove isInSystemHeader() from DiagClient
>> isInSystemHeader really is a hack and it's been on my todo list to  
>> fix for quite awhile.  However, I think it is best to turn it into  
>> a property managed by SourceManager on a per-file-id basis.  I  
>> think that when the preprocessor ends up calling  
>> "SourceManager::create*FileID", that it should pass in the system- 
>> headerness into SourceManager.  Then we'd have methods on  
>> SourceManager that tell you if a SourceLocation is in a system  
>> header or not.
>> Does this seem reasonable?
> That's a great idea. I've done that.

Excellent!  Some minor tweaks:

  unsigned SourceManager::createFileID(const ContentCache *File,
+                                     SourceLocation IncludePos,
+                                     bool isSysHeader
+                                     ) {

Please put the ") {" on the previous line for consistency with the  
rest of the code.

+    /// isSystemHeader - Set for system header files.
+    bool isSysHeader;

Two things:  1) I think it makes sense to preserve the "is extern C  
system header" bit as well that is tracked by HeaderSearch.  2) you  
can steal some bits from ChunkNo (changing it into a 30-bit bitfield)  
to avoid increasing the size of FileIDInfo.  #1 will be important when  
C++ support comes up far enough to start caring about implicit extern C.

TextDiagnostics disappearing is very nice!

Please commit with these tweaks, thanks Nico!


More information about the cfe-dev mailing list