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

[Ted Kremenek]

On Aug 8, 2008, at 11:54 AM, Nico Weber wrote:

> Hi,
>
> On 08.08.2008, at 01:35, Ted Kremenek wrote:
>>
>> HeaderSearch is tied to the Preprocessor, and hence belongs in  
>> libLex, not libBasic.
>
> Why? HeaderSearch does not use anything from the Preprocessor. To  
> me, it's not more Lex-y than IdentifierInfo, which is in Lex, too.

HeaderSearch implements functionality of the Preprocessor.  It  
provides the logic to help resolve what a #include and #import does.   
It doesn't use the Preprocessor, but it is a piece of the preprocessor  
component.  The whole notion of a system header, etc., is all tied to  
the preprocessor.  That's why I don't believe HeaderSearch belongs in  
libBasic.

>> 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.

I don't see the advantage of having Report methods that are overloaded  
in this manner.  This would also place a burden on clients of  
Diagnositcs to use either method, which seems to shift some of the  
burden of responsibility from DiagnosticClient to the clients of  
Diagnostics.  The clients of Diagnostic should *not* decide the policy  
of how diagnostics are rendered; that's the task of the  
DiagnosticClient.  Clients should typically only care about  
SourceLocations; that's the beauty of the current interface.  Specific  
instances of DiagnosticClient can then decide how diagnostics are  
presented by using information from HeaderSearch, etc.

>> With this change, DiagClient doesn't need to know about  
>> HeaderSearch at all (but Diagnotic must know about it, hence  
>> HeaderSearch would have to stay in Basic).

I agree that methods such as TextDiagnosticClient::isInSystemHeader()  
should be refactored for use by multiple DiagnosticClients, but I'm  
not convinced (yet) that putting it into Diagnostics is the answer.   
They current layering allows the Diagnostics class to be useful even  
in the complete absence of preprocessor information.