[cfe-dev] Tidy vs Warnings: missing override

[David Blaikie via cfe-dev]

On Mon, Aug 20, 2018 at 1:16 PM Matthieu Brucher <matthieu.brucher at gmail.com>
wrote:

> Le lun. 20 août 2018 à 21:11, David Blaikie via cfe-dev <
> cfe-dev at lists.llvm.org> a écrit :
>
>>
>>
>> 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 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.


> 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).
>
>
>>
>>
>>> (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
>>>
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>
>
> --
> Quantitative analyst, Ph.D.
> Blog: http://blog.audio-tk.com/
> LinkedIn: http://www.linkedin.com/in/matthieubrucher
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20180820/50b7eac3/attachment.html>