[cfe-dev] Tidy vs Warnings: missing override

David Blaikie via cfe-dev cfe-dev at lists.llvm.org
Mon Aug 20 13:10:45 PDT 2018


On Sat, Aug 18, 2018 at 11:47 AM JVApen via cfe-dev <cfe-dev at lists.llvm.org>
wrote:

> Hi all,
>
> It looks like I did not get the responses of this thread in my mailbox.
> Let me try to pick in on:
> http://lists.llvm.org/pipermail/cfe-dev/2018-August/058861.html and
> http://lists.llvm.org/pipermail/cfe-dev/2018-August/058903.html
>
> 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.
> (PS: This nuance ain't clear from reading
> http://clang.llvm.org/docs/DiagnosticsReference.html#id300 or
> https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-override.html
> )
>
> From a personal perspective, I enable -Werror and -Weverything.
> 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.
> 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.
>

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).


> (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.)
>

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)


> 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.
> 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.
> 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?
>

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)).

- Dave


>
> Thanks!
> PS: I could not find the code on github where the warning was actually
> triggered.
>
> On Thu, 9 Aug 2018 at 19:26, JVApen <JVApen at gmail.com> wrote:
>
>> Hello all,
>>
>> 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.
>>
>> 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?
>>
>> Tnx
>>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20180820/fde489ef/attachment.html>


More information about the cfe-dev mailing list