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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a subscriber: dfaure-kdab.
aaronpuchert added a comment.

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? 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.

> 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.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101566