<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Mar 3, 2014 at 5:12 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 15:34, Alexander Kornienko wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">
<br>
On Mon, Mar 3, 2014 at 4:02 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 03/03/2014 12:53, Alexander Kornienko wrote:<br>
<br>
On Sun, Mar 2, 2014 at 5:59 PM, Alp Toker <<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a><br></div>
<mailto:<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><div><div class="h5"><br>
<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) {<br>
Context = Ctx; }<br>
/// \brief Add a diagnostic with the check's<br>
name.<br>
- DiagnosticBuilder diag(SourceLocation Loc,<br>
StringRef<br>
Description);<br>
+ DiagnosticBuilder diag(SourceLocation Loc,<br>
StringRef<br>
Description,<br>
+ DiagnosticIDs::Level Level =<br>
DiagnosticIDs::Warning);<br>
<br>
<br>
Could you order the parameters Loc, Level,<br>
FormatString and<br>
drop the default argument?<br>
<br>
That'll provide visual consistency with the output as<br>
well as<br>
internal consistency with clang's own<br>
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<br>
Level<br>
_after_ the Message clang-tools-extra, and diag(Loc,<br>
"message")<br>
without specifying a Level. Neither looks like a good idea<br>
but if<br>
the plan is to keep that convention then I guess your<br>
patch is OK.<br>
<br>
It's a failing of clang's diag/tablegen system that it<br>
wasn't made<br>
reusable and ended up getting re-rolled in external<br>
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<br>
can come up with a proper interface to manage diagnostic ID<br>
spaces, so that tablegen'd diagnostic tables can be registered<br>
dynamically, I'd be happy to clean up clang-tidy and static<br>
analyzer to use this system.<br>
<br>
<br>
Right on.<br>
<br>
<br>
In the simplest case, we'd need some analogues for struct<br>
StaticDiagInfoRec and maybe struct StaticDiagCategoryRec and a<br>
method to register a block of them and return the diagnostic<br>
ID of the first element, so that the client code could use it<br>
as an offset to the local static IDs.<br>
<br>
<br>
Agree, but with one significant distinction: It's those<br>
"analogues" that created this problem in the first place where we<br>
have essentially two parallel diagnostic systems in clang plus<br>
another one currently being developed in LLVM core. <br>
<br>
We really need to peel things back at this point so structures<br>
like StaticDiag*Rec are shared by built-in and custom diagnostic<br>
code paths instead of duplicated.<br>
<br>
<br>
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.<br>
</div></div></blockquote>
<br>
There's no cost in exposing StaticDiagInfoReg and sharing the structure for now.<br>
<br>
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.<br>
<br>
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.<br>
<br>
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.</blockquote><div><br></div>
<div>I guess, SFINAE was hard-coded, because it changes significantly how the diagnostics are processed. I guess, Richard can tell more about this.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class=""><br>
<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<br>
straightforward as moving them out of the cpp to a .h file and<br>
taking it from there. Ditto getting diag td files to include<br>
"Diagnostic.td" instead of the reverse that exists now --<br>
otherwise maintaining out-of-tree diagnostic tds is a non-started.<br>
<br>
<br>
As for registering blocks of diagnostic IDs, that's been kind of<br>
unpleasant and doesn't scale well to plugins and external projects.<br>
<br>
<br>
Can you explain why?<br>
</blockquote>
<br></div>
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?<br></blockquote><div><br></div><div>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.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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. </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class=""><br>
<br>
<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<br>
hash of the Rec contents that'll be computed by TableGen at<br>
compile time, as well as optionally at runtime for diagnostics<br>
that need to be defined dynamically. Should solve various quirks<br>
that are getting swept under the carpet today.<br>
<br>
<br>
What are you going to do with collisions?<br>
</blockquote>
<br></div>
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.</blockquote><div><br></div><div>Are you really talking about being able to hash values (some dynamically, some in run-time) without collisions?</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="HOEnZb"><font color="#888888"><br>
<br>
Alp.</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
<br>
<br>
<br>
<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>
<br>
<br>
Keen to go ahead with it so long as we're treating the work as<br>
much-needed cleanup, with clang-tools-extra getting the<br>
functionality when it's ready. It is a non-trivial but the<br>
reduction in cruft and will surely pay off.<br>
<br>
<br>
Alp.<br>
<br>
-- <a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts<br>
<br>
<br>
<br>
</blockquote>
<br>
-- <br>
<a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts<br></div></div></blockquote></div>
</div></div>