This moves some includes missed in D133212.
- Group Reviewers
- rG040f50ad6bad: [NFC][libc++] Moves transitive includes location.
Why would we want to do this? We don't include the normal headers outside the include guard, and AFAICT the only thing this changes is that the compiler potentially has to load the files more often just to do nothing. That shouldn't be a problem, since the compiler optimizes that AFAIK, but why even open the possibility?
Reasons for doing it this way:
- Less chance of depending on transitive includes without realizing it
- This can solve circular dependency issues. For example, if <vector> uses std::copy via <algorithm> but <algorithm> also uses std::vector via <vector> (to implement something unrelated to std::copy), including <algorithm> at the top of <vector> will cause an error, because you'll parse the top of <vector>, then all of <algorithm>, and you'll fail to find a declaration of std::vector. If you do it this way, you'll parse all of <vector>, and then at the end you'll include <algorithm>, which will try to include <vector> again without success, but you'll have already defined std::vector so it's fine.
I think you misunderstood what I'm questioning. I'm not complaining about moving the includes to the bottom of the header - that's a good idea; I'm complaining about moving them outside the include guard. I don't see and benefit in doing that, but it might cause the compiler to load the files more often than necessary, since the include guard doesn't cover them anymore.
Oh. I must say I don't remember why and don't understand why I left the original comment that triggered this change: https://reviews.llvm.org/D133212#inline-1285216. I must have thought that keeping the includes inside the header guards would mean that the cyclic includes problem isn't fixed, which isn't true.
Regardless, I agree that this would probably inhibit the header guard optimization from triggering, which would be rather bad. I think we should keep the parts of this change that move transitive includes from the top to the bottom of the header, but drop the parts that move them outside of the include guards. Sorry for my misleading comment on D133212.
Also, that wasn't necessarily clear from the previous interactions on this patch, but I originally thought this was just moving stuff from the top to the bottom of includes -- it didn't click that the patch was actually moving stuff from inside to outside the header guards.