[PATCH] Add a level parameter to ClangTidyCheck::diag.
Richard Smith
richard at metafoo.co.uk
Mon Mar 3 13:16:20 PST 2014
On Mon, Mar 3, 2014 at 12:00 PM, Alp Toker <alp at nuanti.com> wrote:
>
> On 03/03/2014 18:28, Alexander Kornienko wrote:
>
>> On Mon, Mar 3, 2014 at 5:12 PM, Alp Toker <alp at nuanti.com <mailto:
>> alp at nuanti.com>> wrote:
>>
>>
>> On 03/03/2014 15:34, Alexander Kornienko wrote:
>>
>>
>> On Mon, Mar 3, 2014 at 4:02 PM, Alp Toker <alp at nuanti.com
>> <mailto:alp at nuanti.com> <mailto:alp at nuanti.com
>> <mailto: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>
>> <mailto:alp at nuanti.com <mailto:alp at nuanti.com>>
>> <mailto:alp at nuanti.com <mailto:alp at nuanti.com>
>>
>> <mailto: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.
>>
>>
>> There's no cost in exposing StaticDiagInfoReg and sharing the
>> structure for now.
>>
>> It's conceivable some Sema plugin will want to emit SFINAE
>> diagnostics just as you needed access to WarningOptions but found
>> it to be missing.
>>
>> On the other hand there is a cost in maintaining parallel
>> structures that are "similar but slightly different" because each
>> needs separate code paths for handling and emission.
>>
>> And it's also not clear why SFINAE was hard-coded into the
>> structure in the first place -- it could just as well be
>> represented as a regular diag group. A quick cleanup would resolve
>> this.
>>
>>
>> I guess, SFINAE was hard-coded, because it changes significantly how the
>> diagnostics are processed. I guess, Richard can tell more about this.
>>
>
> SFINAE errors don't affect diagnostic processing other than one place in
> Sema so this is just another predicate. Groups could be made to represent
> these and it might be a nice cleanup.
>
A diagnostic can only be in one group (which can itself be in other groups,
but...) so this model would be extremely awkward to work with. Diagnostic
groups are intended to capture the semantics of -W flags; it doesn't seem
obvious that using them for other things (where we really just want a flag
on each diagnostic) would be a good idea.
> For the time being though those SFINAE bits are harmless. They already
> exist unset in a few hundred built-in diagnostic kinds so it's not a big
> deal to include them in custom diagnostics.
>
> And anyway, it'll be transparent because clang-tools-extra will just
> define diagnostic kinds using ordinary tablegen syntax without having to
> know that they're stored as StaticDiagInfoRecs.
>
>
>
>
>> 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?
>>
>>
>> Well, why reserve a few hundred IDs here and there, some
>> statically, some dynamically that may or may not be used if we
>> don't need to?
>>
>>
>> I was talking about leaving core diagnostics be statically allocated, and
>> all external diagnostics be dynamically registered in the runtime. There's
>> no need to pre-allocate ID spaces. This would require having a single
>> global variable per diagnostics table - the offset of the table in the
>> global diagnostic ID space. But all the diagnostic IDs will continue to be
>> sequential.
>>
>> Hashing will probably simplify the implementation by not requiring
>> upfront decisions on how many diagnostic IDs to pre-allocate to
>> whom, especially for things like plugins where we don't know the
>> need.
>>
>>
>> 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?
>>
>>
>> With categories included in the hash they won't collide unless
>> there's a genuine bug like multiple initialization or mistakenly
>> duplicated diagnostics. Which is a neat property.
>>
>>
>> Are you really talking about being able to hash values (some dynamically,
>> some in run-time) without collisions?
>>
>
> Could do. Sequential IDs with a offsets also work :-)
>
>
> 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/1693bd11/attachment.html>
More information about the cfe-commits
mailing list