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

[Nico Weber]

Hi,

On 08.08.2008, at 01:35, Ted Kremenek wrote:

> On Aug 7, 2008, at 12:17 PM, Nico Weber wrote:
>
>> Hi,
>>
>> the TextDiagPrinter in libDriver causes a crash if one forgets to  
>> (or doesn't know that one has to) call setHeaderSearch() on it. A  
>> simple fix is to check if headersearch is valid in  
>> TextDiagnostics::isInSystemHeader(). However, this really is a  
>> hack: A DiagClient without a HeaderSearch is always a rogue object,  
>> since it doesn't implement isInSystemHeader() correctly (case in  
>> point: Running clang with `-html-diags=html` produces a different  
>> set of warnings as running clang without that option.
>>
>> So I propose a HeaderSearch object should always be mandatory for a  
>> DiagClient (which means HeaderSearch, HeaderMap and DirectoryLookup  
>> move from Lex to Basic). Then, isInSystemHeader() can be  
>> implemented in the base DiagClient. FormatDiagnostic() should also  
>> be there; most clients need this anyways.
>>
>> In return, DiagClient should be optional for Diagnostic. If the  
>> client is 0, then no diagnostics are reported (the ErrorOccurred  
>> flag is of cause still set). That this is the "right thing" is  
>> already hinted at by the change Ted landed today that change  
>> DiagClient from a reference to a pointer. This also makes  
>> constructing a Preprocessor object easier.
>>
>> The attached patch implements the proposed changes. This removes  
>> some code duplication from PathDiagnostic, and with this `-html- 
>> diags` outputs the same warnings as command-line clang. If clients  
>> _want_ to see warnings in system headers, they can still override  
>> isInSystemHeader() to always return false.
>>
>> Comments? :-)
>
> Hi Nico,
>
> I like the idea of where this is going.  A few comments:
>
> (a) HeaderSearch doesn't belong in libBasic.
>
> 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.

> (b) DiagnosticClient should not always require a Preprocessor/ 
> HeaderSearch.
>
> Consider the case where we use serialized ASTs as opposed to the  
> original source.  While all parsing/Sema related diagnostics will be  
> emitted when we have a Preprocessor object, later tasks that operate  
> equally well on serialized ASTs or freshly parsed source (e.g., code  
> generation) may not have a Preprocessor or a HeaderSearch  
> available.  I'm not certain what the operating model should be here,  
> but it seems to be that DiagnosticClients should be able to provide  
> some functionality in the absence of HeaderSearch.

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.

Then, the Report() function would not need to get a FullSourceLoc and  
SourceRanges either; they only make sense if a HeaderSearch is  
available.

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

How does that sound?

Nico