[cfe-dev] Tidy vs Warnings: missing override

[Matthieu Brucher via cfe-dev]

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.
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/32f91d82/attachment.html>