[PATCH] Add a level parameter to ClangTidyCheck::diag.

[Alp Toker]

On 02/03/2014 16:18, Alp Toker wrote:
>
> On 02/03/2014 12:25, Peter Collingbourne wrote:
>>
>> Index: clang-tidy/ClangTidy.h
>> ===================================================================
>> --- clang-tidy/ClangTidy.h
>> +++ clang-tidy/ClangTidy.h
>> @@ -76,7 +76,8 @@
>>     void setContext(ClangTidyContext *Ctx) { Context = Ctx; }
>>       /// \brief Add a diagnostic with the check's name.
>> -  DiagnosticBuilder diag(SourceLocation Loc, StringRef Description);
>> +  DiagnosticBuilder diag(SourceLocation Loc, StringRef Description,
>> +                         DiagnosticIDs::Level Level = 
>> DiagnosticIDs::Warning);
>
> Could you order the parameters Loc, Level, FormatString and drop the 
> default argument?
>
> That'll provide visual consistency with the output as well as internal 
> consistency with clang's own getCustomDiagID(Level L, StringRef 
> FormatString).
>
> That way it becomes kind of a shorthand for diag(getCustomDiagID(...)) 
> << ... which is a step towards unifying built-in and custom diagnostic 
> IDs.

So it looks like there's a convention of listing the diag Level _after_ 
the Message clang-tools-extra, and diag(Loc, "message") without 
specifying a Level. Neither looks like a good idea but if the plan is to 
keep that convention then I guess your patch is OK.

It's a failing of clang's diag/tablegen system that it wasn't made 
reusable and ended up getting re-rolled in external projects, each with 
slightly different interfaces :-/

Alp.

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