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

aaronpuchert abandoned this revision. aaronpuchert added a comment. In 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 t

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

aaron.ballman added a comment. In 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 questi

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

aaronpuchert added a reviewer: aaron.ballman. aaronpuchert added a subscriber: aaron.ballman. aaronpuchert added a comment. 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 quest

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

aaronpuchert added a comment. In D101566#2891972 , @dblaikie wrote: > 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 Right, he didn't suggest this particular fix but ano

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

dblaikie added a comment. In D101566#2891923 , @aaronpuchert wrote: > In D101566#2891764 , @dblaikie > wrote: > >> This patch is still conflating two things - effectively removing an existing >> warning (which

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

aaronpuchert added a subscriber: dfaure-kdab. aaronpuchert added a comment. In 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

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

dblaikie added a comment. In D101566#2891699 , @aaronpuchert wrote: > @dblaikie, does https://bugs.llvm.org/show_bug.cgi?id=18733#c17 or the > following comment change anything about your position? No, not really. This patch is still conflating two th

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

aaronpuchert added a comment. @dblaikie, does https://bugs.llvm.org/show_bug.cgi?id=18733#c17 change anything about your position? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101566/new/ https://reviews.llvm.org/D101566

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

dblaikie added a comment. In D101566#2786112 , @aaronpuchert wrote: > In D101566#2785271 , @dblaikie > wrote: > >> Right - to remove -Wweak-template-vtable in its entirety. The original >> implementation explic

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

aaronpuchert added a comment. In D101566#2785271 , @dblaikie wrote: > Right - to remove -Wweak-template-vtable in its entirety. The original > implementation explicitly didn't warn on implicit instantiations and I think > the fact that it warned on expl

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

dblaikie added a comment. In D101566#2785190 , @aaronpuchert wrote: > In D101566#2734948 , @dblaikie > wrote: > >> Makes it hard to justify the complexity in the compiler if it's hard to >> justify/support the

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

aaronpuchert added a comment. In D101566#2734948 , @dblaikie wrote: > Makes it hard to justify the complexity in the compiler if it's hard to > justify/support the value of the warning. The complexity for `-Wweak-template-vtables` is just 10 lines of co

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

dblaikie added a comment. In D101566#2733172 , @aaronpuchert wrote: > In D101566#2730746 , @dblaikie > wrote: > >> Out of curiosity - have you tried it & measured any significant >> improvement/value in build t

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

aaronpuchert added a comment. In D101566#2730746 , @dblaikie wrote: > Out of curiosity - have you tried it & measured any significant > improvement/value in build times/sizes/etc? No, I fear that would take too much time. (Not so much the benchmarking,

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

dblaikie added a comment. In D101566#2730555 , @aaronpuchert wrote: > So I tried this in two code bases, both of which don't have `-Wweak-vtables` > enabled though. > > In the first (~500 TUs) there were a couple of interesting finds. But they > are ha

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

aaronpuchert added a comment. So I tried this in two code bases, both of which don't have `-Wweak-vtables` enabled though. In the first (~500 TUs) there were a couple of interesting finds. But they are hard to fix, even where explicit instantiations were already there: the corresponding instan

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

aaronpuchert updated this revision to Diff 341845. aaronpuchert added a comment. - Fix punctuation in expected warning message. - Fix test failure: the instantiated class might not be a template, it could also be the member of another template. Repository: rG LLVM Github Monorepo CHANGES SIN

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

aaronpuchert added a comment. In D101566#2727312 , @dblaikie wrote: > I think it'd be good to gather that data first before committing it to Clang > - that's usually what we try to do to justify the warning. Well, except that this isn't a new warning. B

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

dblaikie added a comment. In D101566#2727244 , @aaronpuchert wrote: > In D101566#2726820 , @dblaikie > wrote: > >> Along time ago Clang had a fairly strong aversion to implementing "off by >> default" warnings

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

aaronpuchert added a comment. In D101566#2726820 , @dblaikie wrote: > Along time ago Clang had a fairly strong aversion to implementing "off by > default" warnings ([...]) because they would tend to go unused and > unmaintained. That was a valid reason

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

dblaikie added subscribers: respindola, doug.gregor. dblaikie added a comment. Along time ago Clang had a fairly strong aversion to implementing "off by default" warnings (though clearly weak-vtables was an exception to that - it's a pretty esoteric warning even at the best of times/without this

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

aaronpuchert created this revision. aaronpuchert added reviewers: dblaikie, rsmith. aaronpuchert requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Implicit instantiations of template classes with virtual methods might cause the vtable (and al