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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
