rsmith added a comment.
I would feel a lot more comfortable adding a `-wtest` flag if we had more than
one warning that it would control. If there's really only one warning out of
>600 that qualifies for this treatment, I really don't think we have a good
argument to introduce a new feature here, and it'll just be dead weight we're
carrying forever. My inclination is to say that the single `-Wno-` flag is
sufficient until we actually have multiple warnings that this would control,
and this is a premature generalization until that point. ("We're just about to
add these warnings" carries a lot more weight for me here than "We have ideas
of warnings we might add", but if we're in the former situation, there seems to
be no harm in waiting.)
I'm also concerned about the deployability and utility of this feature.
Identifying "test code" is not going to be easy in a lot of build systems. Even
if successfully deployed, this would mean that refactorings moving code between
files (eg, out of test code into shared infrastructure code) could affect
whether clang accepts the program (eg, under `-Werror`), which seems pretty
undesirable. And likewise, tests that check that (for instance) certain macro
expansions produce valid code would stop being reliable -- the expansion might
be valid in test code but not valid in non-test code. But for me that's a minor
concern at worst: if there are users who are happy with the tradeoffs here, I'd
be OK with us carrying support for them. The major concern here is: are there
users who would enable this feature? (Briefly taking off my Clang hat and
putting on my Google hat, I think we -- as the people who originally had major
problems with the expanded `-Wself-assign` warning -- would be very unlikely to
ever use `-wtest` because of the refactoring issue.)
Finally, let's assume that this is successful and we end up with dozens of
warnings covered by `-wtest`. How confident are we that we're not going to want
a case-by-case way to turn them back on? That is, how sure are we that this
isn't just another warning group (albeit one that only really makes sense to
turn off, not to turn on)? For `-w`, this isn't really a concern, because `-w`
is very much a "just compile it, I do not care about bugs or code quality" flag
which doesn't really make sense to only partially deploy.
================
Comment at: include/clang/Basic/Diagnostic.td:102-104
+class ShowInTests {
+ bit HideInTests = 0;
+}
----------------
This does not seem like it should ever be necessary.
Repository:
rC Clang
https://reviews.llvm.org/D45685
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
