Tidy vs Warnings: missing override

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

Hi,

I experience the override-warning when it was inconsistent within the class (meaning it has been used at least once and some places missed it). This measure is probably to not warn on old code bases.

Could this be the case?

Best, Jonas

Hi,

I have noticed the missing override warning when using virtual diamond inheritance. In that case, there was no warning generated.

Regards,

Matthieu

That would be worth a bug-report :slight_smile:

Yep, that’s the general idea - the bar for Clang warnings has generally (with some variation) been to have a low false-positive rate (where a false-positive is “warns the user even though there is no behavioral bug (or significant risk of confusion from readers of the source code) in the code”) - so a blanket warning on all missing overrides wouldn’t meet that bar - lots of code gets along fine without them. But if the user wrote at least one ‘override’ keyword in a class, fair bet they meant to write them all, so warning in that case seems feasible.

clang-tidy is more stylistic & has a lower bar (one of the reasons for the different bar is that clang-tidy’s warnings don’t impact the clang codebase in both code complexity and performance - which is a reason we can’t implement all the possible warnings in there).

  • Dave

Yes, I have to write an example case!
I think the odds of an error with a wrong override are very low. Ont he contrary, I noticed several times errors with missing overrides and APIs moving and wrong arguments.

Cheers

When I say a “false positive” in this context it’s not a case where a warning would suggest override where it would be invalid/incorrect to put ovterride - but that it would suggest the ‘override’ keyword when it would be benign/when it isn’t telling the user there’s a bug/mistake here.

On an old codebase, likely /many/ more missing overrides are there because there was no override keyword when the code was written - most of those missing overrides aren’t buggy code. So this warning would cause a lot of cleanup for relatively little bug finding.

  • Dave

I think that the override is not an after-the-bug tool, but really a proactive checker. It doesn’t tell you of a bug, it prevents you for introducing one.
On an old code base, that’s where I find it the most helpful, when you have to work on it. Without the override, you don’t have the certainty of properly updating the APIs (old code base also often miss proper test coverage). With override, before your code changes, you can ensure that you are refactoring the full API.

Cheers,

Matthieu

I think that the override is not an after-the-bug tool, but really a proactive checker.

Right - and this is why it’s probably not suitable as a warning, and better suited to being in clang-tidy.

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

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?

Thanks!
PS: I could not find the code on github where the warning was actually triggered.

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

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

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

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.