[PATCH] Allow overriding flag names in diagnostics.

[Alp Toker]

On 22/04/2014 00:26, Diego Novillo wrote:
>
>
>
> On Mon, Apr 21, 2014 at 7:20 PM, Alp Toker <alp at nuanti.com 
> <mailto:alp at nuanti.com>> wrote:
>
>
>         @@ -1084,15 +1100,22 @@
>             return DB;
>           }
>           -inline DiagnosticBuilder
>         DiagnosticsEngine::Report(SourceLocation Loc,
>         - unsigned DiagID) {
>         +inline DiagnosticBuilder
>         +DiagnosticsEngine::Report(SourceLocation Loc, unsigned
>         DiagID, StringRef Val) {
>             assert(CurDiagID == ~0U && "Multiple diagnostics in flight
>         at once!");
>             CurDiagLoc = Loc;
>             CurDiagID = DiagID;
>         +  FlagNameValue = Val.str();
>             return DiagnosticBuilder(this);
>           }
>
>
>     Did you consider adding a DiagnosticBuilder function to append a
>     FlagNameValue instead of all this?
>
>     That would reduce the impact of your patch and improve readability
>     in the callers. Something like:
>
>       Diag(Loc, ID).extendFlag("=mypass") << D;
>
>
> Yeah, briefly. But it seemed to involve more typing, so I had a slight 
> preference over the shorter version. It would also mean resetting the 
> flag name after each call.
>
> I'm not really opposed to the idea, though. I just found the current 
> version easier to use.

I recommend not modifying the Diag() signature because the feature will 
be unavailable to other modules until they each update their own Diag() 
signatures introducing latent churn. To get an idea of this:

include/clang/AST/CommentLexer.h:  DiagnosticBuilder Diag(SourceLocation 
Loc, unsigned DiagID) {
include/clang/AST/CommentParser.h:  DiagnosticBuilder 
Diag(SourceLocation Loc, unsigned DiagID) {
include/clang/AST/CommentSema.h:  DiagnosticBuilder Diag(SourceLocation 
Loc, unsigned DiagID) {
include/clang/Driver/Driver.h:  DiagnosticBuilder Diag(unsigned DiagID) 
const {
include/clang/Lex/Lexer.h:  DiagnosticBuilder Diag(const char *Loc, 
unsigned DiagID) const;
include/clang/Lex/Preprocessor.h:  DiagnosticBuilder Diag(SourceLocation 
Loc, unsigned DiagID) const {
include/clang/Lex/Preprocessor.h:  DiagnosticBuilder Diag(const Token 
&Tok, unsigned DiagID) const {
include/clang/Parse/Parser.h:  DiagnosticBuilder Diag(SourceLocation 
Loc, unsigned DiagID);
include/clang/Parse/Parser.h:  DiagnosticBuilder Diag(const Token &Tok, 
unsigned DiagID);
include/clang/Parse/Parser.h:  DiagnosticBuilder Diag(unsigned DiagID) {
include/clang/Sema/Sema.h:  SemaDiagnosticBuilder Diag(SourceLocation 
Loc, unsigned DiagID) {
include/clang/Sema/Sema.h:  SemaDiagnosticBuilder Diag(SourceLocation 
Loc, const PartialDiagnostic& PD);
include/clang/Serialization/ASTReader.h:  DiagnosticBuilder 
Diag(unsigned DiagID);
include/clang/Serialization/ASTReader.h:  DiagnosticBuilder 
Diag(SourceLocation Loc, unsigned DiagID);

And there are probably more cases in other LLVM modules where 
DiagnosticBuilders are passed around without access to the original Diag 
function that may wish to use this.

Adding a fluent function to DiagnosticBuilder instead of modifying 
Diag() signatures is the preferable approach.

Alp.

-- 
http://www.nuanti.com
the browser experts