<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Mar 3, 2014 at 4:02 PM, Alp Toker <span dir="ltr"><<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On 03/03/2014 12:53, Alexander Kornienko wrote:<div><div class="h5"><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On Sun, Mar 2, 2014 at 5:59 PM, Alp Toker <<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a> <mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>>> wrote:<br>
<br>
<br>
On 02/03/2014 16:18, Alp Toker wrote:<br>
<br>
<br>
On 02/03/2014 12:25, Peter Collingbourne wrote:<br>
<br>
<br>
Index: clang-tidy/ClangTidy.h<br>
==============================<u></u>==============================<u></u>=======<br>
--- clang-tidy/ClangTidy.h<br>
+++ clang-tidy/ClangTidy.h<br>
@@ -76,7 +76,8 @@<br>
void setContext(ClangTidyContext *Ctx) { Context = Ctx; }<br>
/// \brief Add a diagnostic with the check's name.<br>
- DiagnosticBuilder diag(SourceLocation Loc, StringRef<br>
Description);<br>
+ DiagnosticBuilder diag(SourceLocation Loc, StringRef<br>
Description,<br>
+ DiagnosticIDs::Level Level =<br>
DiagnosticIDs::Warning);<br>
<br>
<br>
Could you order the parameters Loc, Level, FormatString and<br>
drop the default argument?<br>
<br>
That'll provide visual consistency with the output as well as<br>
internal consistency with clang's own getCustomDiagID(Level L,<br>
StringRef FormatString).<br>
<br>
That way it becomes kind of a shorthand for<br>
diag(getCustomDiagID(...)) << ... which is a step towards<br>
unifying built-in and custom diagnostic IDs.<br>
<br>
<br>
So it looks like there's a convention of listing the diag Level<br>
_after_ the Message clang-tools-extra, and diag(Loc, "message")<br>
without specifying a Level. Neither looks like a good idea but if<br>
the plan is to keep that convention then I guess your patch is OK.<br>
<br>
It's a failing of clang's diag/tablegen system that it wasn't made<br>
reusable and ended up getting re-rolled in external projects, each<br>
with slightly different interfaces :-/<br>
<br>
<br>
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.<br>
</blockquote>
<br></div></div>
Right on.<div class=""><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
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.<br>
</blockquote>
<br></div>
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. </blockquote>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
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.</blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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.</blockquote>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
As for registering blocks of diagnostic IDs, that's been kind of unpleasant and doesn't scale well to plugins and external projects.</blockquote><div><br></div><div>Can you explain why?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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.</blockquote>
<div><br></div><div>What are you going to do with collisions?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
What do you think about this?<br>
</blockquote>
<br></div>
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.<div class="HOEnZb">
<div class="h5"><br>
<br>
Alp.<br>
<br>
-- <br>
<a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts<br>
<br>
</div></div></blockquote></div><br><br>
</div></div>