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

[Chris Lattner]

On Aug 9, 2008, at 9:44 AM, Nico Weber wrote:

>> If isInPrivateHeader() is not meaningful for all DiagClients, it
>> should be removed from DiagClient. Perhaps Diag should have two
>> methods, ReportWithFile(), that takes a HeaderSearch, and Report().
>> Then Lex, Parser, and Sema could call this method, and other clients
>> could call the ordinary Report() function.
>
> The attached patch does the following changes:

Hi Nico!

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.

The IdentifierTable.h comment change is fine, plz commit, the  
SourceManager.h change also looks fine.


>
> * 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?

> * Instead, give Diagnostic a few more Report() overloads that take a  
> HeaderSearch as parameter, so that clients calling Report() decide  
> if a HeaderSearch should be used (and, in turn, warning from system  
> headers should be ignored)

If SourceManager tracked this, we wouldn't need this.

> This fixes the following problems:
>
> * -html-diags (and probably others) does now output the same set of  
> warnings as console clang does
> * nothing crashes if one forgets to call setHeaderSearch() on  
> TextDiagnostic
> * some code duplication is removed

I think that making SourceManager handle system-headerness (and having  
the PP pass this info down from headersearch when it is entering  
#includes, etc) would solve this problem in a less invasive way.  Do  
you think this approach would work?

Thanks for working on this, this is a very needed improvement!

-Chris