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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D101566#2730555 <https://reviews.llvm.org/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 hard to fix, even where explicit instantiations were already there: the corresponding instantiation declarations would require additional includes in the headers because many of the templates are constrained, and concepts don't work well with forward declarations. That's also why we had to disable `-Wundefined-func-templates` there, which is otherwise a useful warning.
>
> In the second code base (~30,000 TUs) it looks a lot more useful. There are many occurrences, but deduplicating and sorting by number of files they occur in finds a couple of templates where explicit instantiation could improve compile times and build sizes. To give some examples, its own standard library has instantiations `basic_ios<char, char_traits<char>>` plus the `wchar_t` equivalent or `basic_ostream<char, char_traits<char>>` and so on coming up in most TUs. Instantiating them explicitly would be natural and likely beneficial.

Out of curiosity - have you tried it & measured any significant improvement/value in build times/sizes/etc?

> Overall it's not a warning that I would enable in regular builds, but rather like `-Wweak-vtables` collect the most common occurrences in special runs and do something about them. (The total number of warnings, not deduplicated, runs into the millions for both warnings on the second code base.)

That sort of use case sounds more like what we'd use clang-tidy for these days.

This doesn't sound especially compelling for a warning (& still seems pretty much not what the original weak-vtables warning was intending to do for templates - and an inversion of the weak-template-vtables (& so I think, if we are going to have this new thing as a warning, it should have a different name and the existing name should be removed)).

I still really think the best thing is to delete the existing weak-template-vtables warning entirely.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101566