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!
-Chris