[PATCH] D101566: Let -Wweak-template-vtables warn on implicit instantiations

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D101566#2727244 <https://reviews.llvm.org/D101566#2727244>, @aaronpuchert wrote:

> In D101566#2726820 <https://reviews.llvm.org/D101566#2726820>, @dblaikie wrote:
>
>> Along time ago Clang had a fairly strong aversion to implementing "off by default" warnings ([...]) because they would tend to go unused and unmaintained.
>
> That was a valid reason, but now there are code bases that work with `-Weverything` and disable the warnings they are not interested in.

Sure - though that's likely a pretty small proportion of the userbase/not something we encourage/support/etc. So still not necessarily getting a lot of value from new warnings - especially this one, I'd imagine (I'd expect most users would turn this warning off and never think about it again - the intersection of people using -Weverything (small population) and people who will want to put explicit instantiations of all their templates with vtables (small as well) seems vanishingly small.

>> I'm sort of inclined towards this subset of the warning (either the poorly implemented one originally, or this version) being that sort of category, and that it'd be better to delete it.
>
> We have other warnings that are basically just about "cleaner" object files, like `-Wmissing-prototypes` or `-Wmissing-variable-declarations`. (These enforce adding `static` on functions/variables not previously declared. Though together with `-Wunused-{function,variable}` one could find more unused functions/variables, which could be seen as improving code quality.)

Missing declarations can be helpful for finding errors in source code earlier, rather than getting harder to understand linker errors (if you miss "const" when defining a function and create an overload instead of defining the thing you declared in the header, for instance).

And at least both those flags were probably implemented for GCC compatibility, which I don't think applies to this warning.

>> The warning was originally written to ignore implicit template instantiations - and should've ignored explicit instantiations too, I think.
>
> Looking at the warning name I think you're right. There is no way to make the vtables strong. But I don't think anybody cares about this technicality, the warning is there to reduce compile time and object file sizes.
>
> In fact it's probably not so much about emitting the vtable, but about emitting all virtual functions, even if they end up unused. In the template case this also implies instantiation.
>
>> @aaronpuchert - do you have plans to use this warning, if it's implemented/changed in this way?
>
> Let's say that I'm considering it. I'll need to see how often this fires and whether the explicit instantiations make sense to me.

I think it'd be good to gather that data first before committing it to Clang - that's usually what we try to do to justify the warning.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101566/new/

https://reviews.llvm.org/D101566