[Basic] Include SmallVector.h for Clang <= 5 #60426
Conversation
Clang 5 (possibly earlier) has a bug where it does not merge default template arguments properly for forwarded declarations. Include `SmallVector.h` directly in this case. This will slightly increase compile times, but only for very old compilers and only for files that don't already include `SmallVector.h` (of which there's currently only 23).
|
I found the reverted cases by grepping the log for |
|
@swift-ci please test |
| @@ -449,7 +449,7 @@ int main(int argc, char *argv[]) { | |||
| return 1; | |||
| } | |||
|
|
|||
| SmallVector<std::unique_ptr<SourceEditConsumer>, 32> Consumers; | |||
There was a problem hiding this comment.
The default here would be quite a bit smaller, if it matters.
There was a problem hiding this comment.
I believe the only place it's added to is below. So technically this could be 2 if we really cared, but given this is just a testing tool and not even remotely hot 🤷
bnbarham
force-pushed
the
include-smallvec
branch
from
446d1c1 to
625de39
Compare
Just to be clear, the only changes I made were reverts where there was a "added default size"-like commit after the initial commit. There's likely many places the default size is preferable but I figure this is okay for a starting point 😅 |
With the change to include `SmallVector.h` directly in `LLVM.h` rather than forward declaring in the only case it matters (ie. Clang <= 5), these fixes are no longer needed. Since defaulted version is preferred when there's no better choice (which is presumably the case if that's how they were originally added), use it instead. Some uses were instead changed to add `llvm::` so remove that too.
bnbarham
force-pushed
the
include-smallvec
branch
from
625de39 to
6de34f3
Compare
|
@swift-ci please test |
|
@bnbarham Yes, that’s totally fine! |
|
@bnbarham this seems to change behavior no? Wouldn't it just make sense to get rid of SmallVector<T, N> if N is the default value? These seem to be different values of N. |
|
Yes, but I'm not convinced the old default values were chosen for any particular reason @gottesmm. All these were from commits that came in after a failure on CentOS 7 (and 2 in the last week). Are there any in particular you're concerned about? |
|
As long as that is what they are, I am fine. Thanks! |
|
I went through them all to double check, they all definitely existed as the default-arg case initially and then were fixed in PRs either referencing the linux failure or on "fix rebranch" PRs (I wonder if CentOS 7 was initially brought up on rebranch?). Going to go ahead and merge. |
Clang 5 (possibly earlier) has a bug where it does not merge default
template arguments properly for forwarded declarations. Include
SmallVector.hdirectly in this case. This will slightly increasecompile times, but only for very old compilers and only for files that
don't already include
SmallVector.h(of which there's currently only23).