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

[Alexander Kornienko]

On Mon, Mar 3, 2014 at 4:02 PM, Alp Toker <alp at nuanti.com> wrote:

>
> On 03/03/2014 12:53, Alexander Kornienko wrote:
>
>  On Sun, Mar 2, 2014 at 5:59 PM, Alp Toker <alp at nuanti.com <mailto:
>> alp at nuanti.com>> wrote:
>>
>>
>>     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 :-/
>>
>>
>> Yes, there's a lot to unify and clean up in this area. If we can come up
>> with a proper interface to manage diagnostic ID spaces, so that tablegen'd
>> diagnostic tables can be registered dynamically, I'd be happy to clean up
>> clang-tidy and static analyzer to use this system.
>>
>
> Right on.
>
>
>> In the simplest case, we'd need some analogues for struct
>> StaticDiagInfoRec and maybe struct StaticDiagCategoryRec and a method to
>> register a block of them and return the diagnostic ID of the first element,
>> so that the client code could use it as an offset to the local static IDs.
>>
>
> Agree, but with one significant distinction: It's those "analogues" that
> created this problem in the first place where we have essentially two
> parallel diagnostic systems in clang plus another one currently being
> developed in LLVM core.


> We really need to peel things back at this point so structures like
> StaticDiag*Rec are shared by built-in and custom diagnostic code paths
> instead of duplicated.


I agree. I just doubt that StaticDiagInfoReg in its current form is generic
enough for this purpose. E.g. I strongly doubt, why any plugin or external
project would need the SFINAE field.


> They're generic descriptions after all so it might be as straightforward
> as moving them out of the cpp to a .h file and taking it from there. Ditto
> getting diag td files to include "Diagnostic.td" instead of the reverse
> that exists now -- otherwise maintaining out-of-tree diagnostic tds is a
> non-started.


> As for registering blocks of diagnostic IDs, that's been kind of
> unpleasant and doesn't scale well to plugins and external projects.


Can you explain why?


> My early thought here is to represent diagnostic IDs as a 32-bit hash of
> the Rec contents that'll be computed by TableGen at compile time, as well
> as optionally at runtime for diagnostics that need to be defined
> dynamically. Should solve various quirks that are getting swept under the
> carpet today.


What are you going to do with collisions?


>
>> What do you think about this?
>>
>
> Keen to go ahead with it so long as we're treating the work as much-needed
> cleanup, with clang-tools-extra getting the functionality when it's ready.
> It is a non-trivial but the reduction in cruft and will surely pay off.
>
>
> Alp.
>
> --
> http://www.nuanti.com
> the browser experts
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140303/5aa55e0a/attachment.html>