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

Thu Jul 22 16:57:38 PDT 2021

aaronpuchert abandoned this revision.
aaronpuchert added a comment.

In D101566#2896911 <https://reviews.llvm.org/D101566#2896911>, @aaron.ballman wrote:

> Off-by-default warnings are generally discouraged unless there's a very compelling reason to have them.

There are IMO valuable warnings that can be noisy initially and will probably never be on-by-default, like `-Wmissing-prototypes`. There are controversial warnings like `-Wshadow` (some people actually like shadowing, others do not). There are contradictory warnings like GCC's `-Wswitch-default` and our `-Wcovered-switch-default`. Warnings for compatibility with older standards. Warnings about certain implicit casts that are way too noisy in many code bases, but some might want them. Only having on-by-default warnings is just not realistic, there will always be many exceptions. Partly because the language moves on, but code needs time to catch up.

But in any event, my goal here wasn't to introduce this warning, just to fix it, given that it's there.

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

That was in response to off-by-default warnings remaining unused.

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

Silencing it in problematic cases means never emitting it at all. So you agree with @dblaikie that it should be removed.

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

Likely not.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101566