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

Thu Jul 22 09:33:03 PDT 2021

aaron.ballman added a comment.

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

> Let's add @aaron.ballman to get a third opinion. The discussion is meandering a bit, but you should understand the gist from the first comments or the bug report. The question is whether it's legitimate to fix this warning or whether the only legitimate course of action is to remove it entirely (and possibly re-propose it independently).

A few random thoughts.

> Along time ago Clang had a fairly strong aversion to implementing "off by default" warnings

That's not a long time ago, that's the status quo. Off-by-default warnings are generally discouraged unless there's a very compelling reason to have them. A long time ago we used to be more relaxed about that, but experience taught us that off-by-default warnings are a poor value proposition in general.

> That was a valid reason, but now there are code bases that work with -Weverything and disable the warnings they are not interested in.

-Weverything is not a particularly compelling use case IMO. We tell people not to enable it and expect good things to happen over time.

> Honestly in retrospect I think it was a bug in the warning that should've been fixed by not warning in this case, rather than splitting it out into its own warning group. The warning was originally written to ignore implicit template instantiations - and should've ignored explicit instantiations too, I think.

I tend to agree, but don't have strong opinions on it. If I'm following along properly, my suggestion would be to commit the uncontentious bit (silencing the diagnostic in the problematic cases) in this review. If still desired, we can always start a new review to see if there's sentiment for the new warning behavior (under the existing diagnostic name or under a new one), but hopefully the proposed new behavior would be palatable for an on-by-default warning as that's more compelling than tweaks to an off-by-default warning that the original author isn't sure should even exist.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101566