<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 23 April 2018 at 20:07, John McCall 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><span>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></span><div class="gmail_extra"><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"><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="m_-8213152809041276746m_-171953234041228843gmail-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></span><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></div></div></blockquote><div><br></div><div>Yes, that's fair, but it also applies to some of the other groups I mentioned. Eg, how can a project undo a build system default of -Wall -Wextra, without turning off enabled-by-default warnings that happen to also be in those groups?</div><div><br></div><div>If we want to address the broader problem here, I think we need a more general model for mapping from a sequence of warning flags to a set of enabled/disabled choices for warnings. We could let each warning specify an arbitrary positive unate function over warning flags that determines whether it's enabled, but I expect that'd be a bit too complex for comprehensibility. So I'm not sure what the best way forward is here, but I'd prefer that it's not a one-off special thing with its own unique semantics, at least if it's in the -W namespace. Maybe having a layer of "-w" flags that override the "-W" flags really is the best model.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><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"><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></span></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></div></blockquote><div><br></div><div>I agree with the philosophy here, but I am as yet unconvinced that using different warning flags for test and non-test code is really a best practice. Rather, I think best practice would be to use the exact same compiler configuration for test and production code, so that you're actually testing the thing you intend to deploy.</div><div><br></div><div>For this self-assign warning, if the signal-to-noise ratio is actually high enough to justify having it at all, I think we should have an idiomatic source-level construct to express that the self-assignment is intentional, and we should suggest that construct in the diagnostic. (Currently, "x = *&x;" appears to be the suggestion, but I think that's overly cumbersome; "(x) = (x);" or similar would seem more elegant.)</div><div><br></div><div>I really like the idea of a -Wrecommended.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><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></div></blockquote><div><br></div><div>I don't dispute that there exist build systems in which this can be done easily (though as you note, Bazel is not currently one of them). But there also exist build systems in which test binaries are just binaries, and test code lives alongside the rest of the code with no particular distinguishing features except that it's only linked into tests. My point is that this feature would not be easily deployed by everyone (and possibly not even by most), not that it would not be easily deployed by anyone.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><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><div><br></div><div>That's a fair point. However, if you're using Clang modules and you're building them explicitly, the warning flags for your infrastructure code are the warning flags for the infrastructure target, not those for the user of that target, even under template instantiation. And likewise for our behavior in the C++ Modules TS.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><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><div><br></div><div><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">I don't see how that follows; </span>maybe we're talking past each other? I'm saying:</div><div><br></div><div> * You have a library that exports a macro FOO</div><div> * You want to test that the expansion of FOO is valid</div><div> * So you expand it within your test code</div><div> * That compilation succeeds</div><div><br></div><div>If you use -Wno-noisy-in-tests for your tests, the above does *not* guarantee that the macro is actually usable by production code, even if you're otherwise using the same compiler and flags for your test and non-test code.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><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><div><br></div><div>OK then. If people will actually use this, I don't want to get in the way, even though I think we'd be missing a chance to solve a more general problem. I think my preferred spelling would be -wtest, since we want it to behave as an override to other -W flags, not as a regular last-flag-wins warning flag, and this also avoids the double-negative.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><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></div></blockquote><div><br></div><div>If -wtest forces warnings off in the same way that -w does, then those fine-grained flags presumably won't work, though. That said, I don't think this is a big concern, at least not for -wtest, and I can't predict what other -w flags we'll find we want in the fullness of time.</div></div></div></div>