[cfe-dev] Proposed minor refactoring of the diag subsystem
clattner at apple.com
Sat Aug 9 14:59:37 PDT 2008
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:
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
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
> * 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!
More information about the cfe-dev