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

[Ted Kremenek]

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.  This seems like a good design abstraction that I would  
like to preserve.


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


I do have a couple suggestions to address (a) and (b), and you and  
others are free to disagree with them. (I'm not so sure of them myself)

To address (a), we can create a new libDiagnostic that layers on top  
of libBasic and libLex.  This would be nice because we could also move  
the PathDiagnostic code out of libAnalysis into libDiagnostic, as  
opposed to having Diagnostic and PathDiagnostic in two libraries.  The  
potential negative here is that we get another library.  I'm not  
certain if that is a problem; some may be concerned about a  
proliferation of too many libraries.

To address (b), we can have HeaderSearch be passed as an ctor argument  
to DiagnosticClient, but also allow for HeaderSearch to be NULL.  This  
complicates some of the code for DiagnosticClient, but it allows  
DiagnosticClients to be used when such information isn't available.   
This means that methods like getHeaderSearch() should return a  
HeaderSearch* (which can be NULL) instead of a HeaderSearch&.

Thoughts?

Ted