CXX11 patch to warn if 'override' is missing on overriding virtual function

[David Blaikie]

On Thu, Sep 25, 2014 at 11:24 AM, Douglas Gregor <dgregor at apple.com> wrote:

>
> On Sep 24, 2014, at 4:54 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Wed, Sep 24, 2014 at 4:42 PM, Argyrios Kyrtzidis <kyrtzidis at apple.com>
> wrote:
>
>> This isn’t a style checker, this is to make sure you are not overriding
>> something by accident.
>>
>
> For context, I took those quotations from a thread about a warning for
> "using a null pointer other than nullptr" which seems pretty analogous to
> this warning. "Use this feature because it might help you find bugs" but I
> think Doug was basically saying that the guideline to use a particular
> feature is a stylistic one, even if that style then leads to finding bugs.
>
>
> Yeah, both warnings fall into the “introduce extra syntax, the consistent
> use of which will prevent bugs” camp. One can choose to use the feature if
> the syntactic overhead is less burdensome than tracking down the bugs, or
> not.
>
> My position has softened somewhat, and I don’t feel as strongly that these
> warnings is “purely stylistic”. I still think an off-by-default warning is
> extremely unfortunate, because new warning flags aren’t that discoverable.
> I’d feel a lot better if some part of the warning could be on by default.
> For example, if you’ve uttered “override” at least once in a class, it
> makes sense to warn-by-default about any other overrides in that class that
> weren’t marked as “override”, because you’re being locally inconsistent.
>

I believe James Dennett had the same idea - filed a bug internally which we
never got around to filing externally/fixing. I tend to agree that that
seems like a totally reasonable indicator/signal and "overriding functions
without explicit override in a class with at least one explicit override"
are probably mistakes, if not outright bugs, of some degree that could
reasonably classified as a "true positive".


> Or maybe you can expand that heuristic out to a file-level granularity
> (which matches better for the null point constant warning) and still be
> on-by-default.
>

Same file would be interesting when it's not just the primary source file
but could just be the same header, etc, sounds reasonable to me. (sort of
going to hate to be the guy that adds the first nullptr to a source file &
have the build light up like a christmas tree as every NULL in the file
produces a warning/error, though... )

That might be tricky to introduce another delayed diagnostic machine which
has to wait & see if there's an override/nullptr anywhere in the file, then
go back & emit the delayed diagnostics for this purpose.


> Explicitly providing the warning flag (or some other related warning flag)
> could turn the warning on everywhere, for those who want the rule
> consistently applied to their code base.
>
> - Doug
>
>
>> It needs to be off-by-default due to the vast amount of C++ code that
>> exists out there, but we can have it enabled when you create a new project.
>>
>> On Sep 24, 2014, at 4:16 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>
>>
>> On Wed, Sep 24, 2014 at 3:57 PM, jahanian <fjahanian at apple.com> wrote:
>>
>>> Hi all,
>>>
>>> We have an enhancement request  from our users to provide
>>> warning if ‘override’ control is missing. This warning is off by default
>>> as it will
>>> be noisy (but it will show itself with -Weverything).
>>> Is such a patch useful enough to go into the trunk? Also, comment on the
>>> patch is appreciated.
>>> I will provide ‘fixit’ later if this is a worthwhile patch.
>>>
>>
>> While I rather like the idea of such a warning, the usual bar has been a
>> strong aversion to adding off-by-default warnings. I think the theoretical
>> future might be building warnings like this into clang-tidy, then providing
>> some plugin-like option to enable certain clang-tidy warnings in your
>> normal builds.
>>
>> (because I was curious, I went back & found some choice quotes from Doug
>> Gregor on warnings like this (this is what he told me, years ago, when I
>> proposed adding a warning for null pointers that aren't nullptr*):
>>
>> "Off-by-default warnings are not a mechanism to subvert our normal
>> processes for vetting awarning. In general, we should avoid off-by-
>> default warnings: if it's not good enough to turn on by default, why do
>> we have it at all? The vast majority of users will never see an off-by-
>> default warning."
>> "A compiler is not a style checker, nor should it ever be."
>> "Warnings are intended to find potential problems in the source code.
>> Style migration is the domain of separate tools.")
>>
>> (& cc'ing Doug in case there's something about this that's
>> different/things have changed over the years)
>>
>> * today, I'd probably be able to get that in on the basis of
>> compatibility with GCC's -Wzero-as-null-pointer-constant
>>
>>
>>>
>>> - Fariborz
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140925/5df8e146/attachment.html>