[cfe-dev] Diagnostic Improvements

[Ted Kremenek]

On Nov 16, 2008, at 9:25 PM, Chris Lattner wrote:

>
> I think it is time to stop the madness.  Currently, diagnostics are
> passed down into DiagnosticClient with an array of strings that are
> referenced by substitutions, and turned into the final string by
> DiagnosticClient::FormatDiagnostic.  I think it would be straight-
> forward to change this to be an array of void*, where (at the
> beginning) all of these are pointers to std::string.  Once that was
> done, we can start introducing some more rich formatting characters
> like "%T0" instead of "%0" and making the corresponding pointers be to
> richer thing than strings.  For example, "%T0" format the 0th element
> of the array as a QualType.  We could also introduce things like %u0
> (unsigned int) and %i0 (signed int).

I like this idea.  I think that our diagnostics should be more  
"intentional" in nature and have the localization component handle  
more of the actual text that is rendered.  Another way of phrasing it  
is the component that issues warning should be indifferent on how they  
are presented or rendered (which includes localization).

Another benefit of passing arbitrary objects instead of strings is  
that you have the opportunity (depending on the DiagnosticClient) to  
render key things like function names (e.g., by passing a  
FunctionDecl*) in arbitrary ways.  For example, one can imagine that  
the HTML diagnostics engine (used by the static analyzer, but also  
usable for regular diagnostics) would add hyperlinks from function  
names to their definition.  In a similar way IDEs could leverage such  
information to build truly rich experiences.  By "lowering" to strings  
too early when we generate diagnostics, we take away some of the  
responsibility of "presentation" from the DiagnosticClient.

> Doing this would directly solve #1 above, because 1) diagnostics that
> are squelched never make it to FormatDiagnostic and 2) those that are
> could be printed into one big SmallString.  This would also solve #2,
> because FormatDiagnostic would be given the QualType, so it could do
> any crazy AST-level groveling that it wants to format it based on what
> the DiagClient implementation wants.  This also is a big step to
> solving #3, because it would mean that we could write things like:
>
> DIAG(err_operator_overload_must_be_binary, ERROR,
>      "overloaded '%0' must be a binary operator (has %u1 parameter
> %up1)")

I think that in the end we should probably go farther than this and  
not have any English text in the character string passed to DIAG since  
we have to think about localization from the get-go instead of  
retrofitting it in.  This of course is problematic for custom  
diagnostics.