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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to