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