<div dir="ltr">On Mon, Apr 23, 2018 at 8:23 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span>On 23 April 2018 at 16:23, David Blaikie via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><span class="gmail-m_6927491989279465522gmail-m_8368031217052705392m_7766937186180800223gmail-"><div dir="ltr">On Mon, Apr 23, 2018 at 4:12 PM John McCall <<a href="mailto:rjmccall@gmail.com" target="_blank">rjmccall@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr">On Mon, Apr 23, 2018 at 6:32 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr">On Mon, Apr 23, 2018 at 3:29 PM John McCall via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br><div class="gmail_quote"><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">rjmccall added a comment.<br>
<br>
In <a href="https://reviews.llvm.org/D45766#1076176" rel="noreferrer" target="_blank">https://reviews.llvm.org/D4576<wbr>6#1076176</a>, @dblaikie wrote:<br>
<br>
> Is there anything else in the "-w" namespace other than the literal "-w" so<br>
>  far?<br>
<br>
<br>
No. This would be novel.<br></blockquote><div><br></div></span><div>Ah, I see.</div><span><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">> I mean, I could imagine it might make more sense to default these warnings<br>
>  off & users can turn them on for non-test code, potentially? So<br>
>  "-Wnon-test" might make sense.<br>
<br>
That's an interesting idea, but it's still not a warning group, because you shouldn't get the self-assign warnings unless `-Wself-assign` is enabled.<br></blockquote></span><div><br>You shouldn't?</div></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>I wouldn't think so.  Remember that the goal of the option is to be a single thing that users can add to their unit-test CFLAGS to disable these noisy-in-tests cases.  So if we add an opt-in/experimental `-Wunpredictable-foozits` warning, and it has a unit-test carve-out, passing `-wtest -wno-test` or whatever shouldn't turn on the carved-out special case of `-Wunpredictable-foozits`.</div><div><br></div><div>It's probably not the worst thing to just use a `-W` spelling anyway; not everything in that namespace is (e.g. `-Werror`).  It could be `-Wnoisy-in-tests` and `-Wno-noisy-in-tests`, with a documentation note that `-Wnoisy-in-tests` is just a cancellation of `-Wno-noisy-in-tests` and doesn't actually enable any warnings by itself.  We could have the diagnostic printer add `-Wnoisy-in-tests` to the diagnostic-group annotation for diagnostics that would be suppressed under `-Wno-noisy-in-tests`, analogously to how it adds `-Werror` for diagnostics that have been promoted to an error.</div></div></div></div></blockquote></span><div><br>That sort of sounds pretty plausible to me. Poked Richard about his opinion here too.</div></div></div></blockquote><div><br></div></span><div>This is not the only warning group that has the property that only one of -Wfoo and -Wno-foo seems useful. There are, for instance, plenty of diagnostic groups that make sense to turn on, but not to turn off (eg, -Wno-all, -Wno-extra are largely meaningless), and some that make sense to turn off, but not to turn on (eg, -Wnon-gcc is only intended to be turned off). So I don't think that's as special a property as is being suggested. If someone uses -Wnoisy-in-tests and it turns on all warnings that are noisy in tests, I think they got what they asked for.</div></div></div></div></blockquote><div><br></div><div>The issue there is that -Wnoisy-in-tests is likely to be useful as a cancellation of -Wno-noisy-in-tests, which (as David suggests) might reasonably be set by default by a build system.  That's completely defeated if it potentially enables a bunch of unwanted warnings.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>This is also not the only warning group for which we want diagnostics to be in this group and in some other group or groups. I think the idea to include -Wnoisy-in-tests in the diagnostic output is very interesting, but as a general feature not as a -Wnoisy-in-tests special case -- for example, if I use a binary literal in C++98 mode, I'd like the warning to be tagged with [-Wbinary-literal,-Wc++14-exte<wbr>nsions,-Wgnu], rather than the current [-Wc++14-binary-literal] (an artificial warning group that only exists to work around the inability of our infrastructure to properly represent warnings that are in multiple groups at once). </div></div></div></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><div><br></div><div>As a simple way to get this effect, perhaps we could allow warning groups to be tagged as artificial, and for such warning groups, recurse to the warning groups including them to find the name to use for diagnostics.</div></div><div class="gmail_quote"></div></div></div>
</blockquote></div></div><div class="gmail_extra"><br></div><div class="gmail_extra"><div>That's a really good idea.  I don't think it's a good match for this, but I completely agree that there's a separate problem of presenting artificial diagnostic groups to users.</div><div><br></div><div>I'm going to quote your other post into this thread in the interest of making it easier for posterity to read a unified discussion.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><span style="font-size:12.800000190734863px">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.)</span></blockquote><div><br></div><div>This is fair.  This is certainly an early generalization.  I only think it might not be premature because I think it gives us a very clear message for projects: add `-wtest` when building unit tests and (maybe) unit test infrastructure, and the compiler will try to avoid warning about idioms that don't make sense in normal code but might be sensible in unit tests.  If we wait — even if we're only waiting for a second warning — we're jerking project maintainers around.</div><div><br></div><div>In general, I think we should be doing a lot more to provide clearer "best practices" recommendations to our users about warnings.  We may not be willing to add new warnings to the defaults or `-Wall`, but we could certainly add a curated `-Wrecommended` (just one!) with an understanding that the set of warnings in that group will grow over time, and with explicit documentation that projects should not combine it with `-Werror` in builds where they don't themselves specify an exact compiler version.  If users didn't like our recommendations, they could of course continue to tweak them with the finer-grained controls, or just ignore them entirely.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><span style="font-size:12.800000190734863px">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.</span></blockquote><div><br></div><div>Really?  In my experience, this is an extremely common thing to split out in a build system.  That shouldn't be surprising, since test code often has very different build and/or deployment requirements from the rest of the project.  That's why Bazel, for example, splits out `cc_test` as a completely different top-level declaration.  Now perhaps Bazel isn't a great example for my argument; apparently the underlying machinery is hard-coded to not allow easy customization of how tests are built, at least not separately from anything else.  But that's a rather unique limitation of Bazel and its hard-coded abstractions; a build system written in, say, cmake would make that a project-wide function which can easily be modified to pass an extra option.  (I have no idea how easy it would be for Chromium to do this with gypi; probably very easy if one were willing to change gypi itself and not so easy if one has to maintain its purity as a metabuild system.)</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><span style="font-size:12.800000190734863px">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.</span></blockquote><div><br></div><div>The testing idioms that trigger warnings like this are unlikely to be moved (or be movable) into common infrastructure.  Test infrastructure, maybe, but I don't know why that would be included in non-test code.  For example, this self-assign warning can only be abstracted with a macro or a template, i.e. in code that has to be present in the test's translation unit and which is going to be useless in a non-test translation unit.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><span style="font-size:12.800000190734863px">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.</span></blockquote><div><br></div><div>That's a `-Werror`-like argument.  If you're going to what-if on this level, you can literally never touch the warning code again. :)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><span style="font-size:12.800000190734863px">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.)</span> </blockquote><div><br></div><div>I think individual projects would absolutely use this feature if we made it available and told them that it's supposed to be a stable way of avoiding some false positives in their unit tests.  The warning-by-warning suppression seems like much more of a compiler-group sort of solution, suitable for a team like yours or mine that's exhausted by the idea of making changes to hundreds of different build systems.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><span style="font-size:12.800000190734863px">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.</span></blockquote><div><br></div><div>Well, I think we've already agreed that we'll *also* be providing warning-by-warning suppress flags for the sake of compiler maintenance/deployment groups.  I don't know why an individual project maintainer would ever want that finer-grained control.</div><div><br></div><div>John.</div></div></div>