<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Mon, Aug 20, 2018 at 1:16 PM Matthieu Brucher <<a href="mailto:matthieu.brucher@gmail.com">matthieu.brucher@gmail.com</a>> wrote:<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_quote"><div dir="ltr">Le lun. 20 août 2018 à 21:11, David Blaikie via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>> a écrit :<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Sat, Aug 18, 2018 at 11:47 AM JVApen via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi all,<br><br>It looks like I did not get the responses of this thread in my mailbox.<br>Let me try to pick in on: <a href="http://lists.llvm.org/pipermail/cfe-dev/2018-August/058861.html" target="_blank">http://lists.llvm.org/pipermail/cfe-dev/2018-August/058861.html</a> and <a href="http://lists.llvm.org/pipermail/cfe-dev/2018-August/058903.html" target="_blank">http://lists.llvm.org/pipermail/cfe-dev/2018-August/058903.html</a><br><br>If I understood correctly, clang-tidy warns on all occurrences of missing overrides while the compiler warning suppresses the warning if override doesn't appear in the class.<br>(PS: This nuance ain't clear from reading <a href="http://clang.llvm.org/docs/DiagnosticsReference.html#id300" target="_blank">http://clang.llvm.org/docs/DiagnosticsReference.html#id300</a> or <a href="https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-override.html" target="_blank">https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-override.html</a>)<br><br>From a personal perspective, I enable -Werror and -Weverything.<br>With this, I can disable (and document why) warnings I don't like (as the C++98 compatibility) or suppress the warnings locally when explicitly violating.<br>From this perspective, I agree with Matthieu that this makes sense for every occurrence, especially in old code bases since I want to all hidden cases to be fixed asap.<br></div></blockquote><div><br></div><div>It's still a rather stylistic choice about how you write your C++ code - it has a very low rate of finding bugs on a codebase that isn't already override-clean. (ie: it'll suggest /lots/ of code changes (adding override everywhere) but very few of those highlighted locations will be bugs).</div></div></div></blockquote><div><br></div></div></div><div dir="ltr"><div class="gmail_quote"><div>I disagree. When you change a full codebase, changing for instance an argument to be passed as const ref instead of by value, and you end up missing one, it's quite a big problem.</div></div></div></blockquote><div><br>I don't mean to suggest that that's not a bug, or that the bug is not costly - but that, in a large codebase that isn't using the override keyword already fairly pervasively, on a numbers comparison, most of the warnings would not be pointing to bugs - but to cases where the author knew it was an override but hadn't used the keyword yet.<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_quote"><div>Also adding the override is easily done with clang)today (except in diamond inheritance where it misses everything, still trying to find time to submit the bug).</div></div></div><div dir="ltr"><div class="gmail_quote"><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_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">(I might care less if we manage to setup an automated system for checking the clang-tidy warnings to ensure that override ain't forgotten in new files.)<br></div></blockquote><div><br></div><div>Yeah, I'd love to see better ways of automating clang-tidy into an existing build system for adding new/other diagnostics (likely not suitable for the cases where clang-tidy diagnostics are there because they're too slow for normal compiles, but when they're too stylistic/esoteric for clang, but still fast enough - would be great to be able to still get them as part of the build & have them fail the build under -Werror, etc)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">I also understand the reasoning of Dave for why we have clang-tidy as a separate executable: some checks can take a long time to run or need to keep track of a lot of state.<br>However, if I'm understanding this case correctly, neither of these conditions are met as the compiler code already detects the issue and decides to ignore it since no override is available in that file.<br>If we really care about keeping this distinction, would reporting this case in a separate warning (-Winconsistent-missing-override-pedantic) be a solution both scenarios, similar to -Winconsistent-missing-destructor-override?<br></div></blockquote><div><br>If it were added, yes it'd be under a separate flag (though not likely with the "inconsistent" part of the name - because that's specifically about the current low-false positive filter (where the warning only fires if the missing override is inconsistent with the rest of the class (because there are overrides specified elsewhere in the class)).<br><br>- Dave<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br>Thanks!<br>PS: I could not find the code on github where the warning was actually triggered.<br></div><br><div class="gmail_quote"><div dir="ltr">On Thu, 9 Aug 2018 at 19:26, JVApen <<a href="mailto:JVApen@gmail.com" target="_blank">JVApen@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="auto">Hello all,<div dir="auto"><br></div><div dir="auto">Today we have discovered a case where we did not have a compiler warning for a missing override. To verify, we explicitly enabled the warning with the pragma.</div><div dir="auto"><br></div><div dir="auto">While investigating this in the IDE, we noticed that the Visual Assist plugin did notice that missing override. As this uses clang-tidy to determine this, we were wondering if these differences we noticed are intentional. If so, is their any documentation on these differences?</div><div dir="auto"><br></div><div dir="auto">Tnx</div></div>
</blockquote></div>
_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a><br>
</blockquote></div></div>
_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a><br>
</blockquote></div></div><div dir="ltr"><div><br></div>-- <br><div dir="ltr" class="m_-5391081038876564910gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div>Quantitative analyst, Ph.D.<br>Blog: <a href="http://blog.audio-tk.com/" target="_blank">http://blog.audio-tk.com/</a><br>LinkedIn: <a href="http://www.linkedin.com/in/matthieubrucher" target="_blank">http://www.linkedin.com/in/matthieubrucher</a></div></div></div></div></div></div></blockquote></div></div>