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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

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

> In D101566#2891764 <https://reviews.llvm.org/D101566#2891764>, @dblaikie wrote:
>
>> This patch is still conflating two things - effectively removing an existing warning (which I agree with) and adding a new one (which I think is questionable at best - but in any case should be evaluated on its own merits [...], not in relation to the existing broken warning).
>
> There are different ways to look at the situation. I understand your argument, but we have a bug report against this warning, and everyone we've heard who wanted to use the warning seems to have wanted what it does after this fix. (I'm including the original two commenters on the bug, @respindola and @dfaure-kdab.) That is an argument to just fix the warning and let it do what they expected.
>
> If those people thought the warning shouldn't exist at all, why were they using it?

I assume the only way they came across it was because of using -Weverything.

> Or if they thought the name was misleading, why didn't their comments say that? The comments all said that they think it does the wrong thing.

For Rafael - probably because he didn't look at all the cases the warning does catch & see that it's pretty much entirely no use - his suggestion was to only detect/warn on explicit instantiations in headers (where they could produce duplication), which would still be a subset of the existing warning behavior & a subset that's actionable at least. That's different from your proposal to invert the warning, which I think is quite different & not suitable to tie together like this.

>> if we did that, we might consider generalizing it beyond only templates with vtables, for instance (why are they more deserving of explicit instantiations than other templates?)
>
> They behave differently:
>
> - For template classes without vtable, instantiating the class doesn't instantiate any member functions, which are instantiated when odr-used.
> - For template classes with vtable, instantiating the constructor requires instantiating the vtable, which requires instantiating all virtual members.

Lots of classes have many non-virtual functions in addition to the virtual ones - I'd bet on average probably significantly more, and the explicit instantiation will instantiate all those non-virtual ones too, which might not be what the user wants if they similarly don't want to explicitly instantiate their non-polymorphic templates.

> The situation is similar to non-templates actually. If there is no vtable, we can emit functions on demand, but if there is one, we have to emit all virtual functions if we need the constructor, and if there is no key function.
>
>> [...] and potentially not to reuse the same warning flag.
>
> My point is though, and the Bugzilla commenter seems to agree, that both address the same issue for different entities: one for non-template classes, the other for template classes. So I think the name is absolutely fine.
>
> As I've pointed out earlier, disruption isn't a good argument for separately removing this warning and adding another: removing the warning will be disruptive for users of -Weverything, while for obvious reasons we don't expect anyone to have it active and care about it, as it doesn't really make sense right now.

-Weverything is not intended for this use ( https://quuxplusone.github.io/blog/2018/12/06/dont-use-weverything/ for a good write up and various other comments on the issue) - breaking -Weverything users is to some degree a good thing: it's a reminder that what they're doing is unsupported and discouraged.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101566