On Mon, Mar 3, 2014 at 4:02 PM, Alp Toker <[email protected]> wrote: > > On 03/03/2014 12:53, Alexander Kornienko wrote: > > On Sun, Mar 2, 2014 at 5:59 PM, Alp Toker <[email protected] <mailto: >> [email protected]>> 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 > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
