[libcxx-commits] [PATCH] D135180: [NFC][libc++] Moves transitive includes location.

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

In D135180#3835058 <https://reviews.llvm.org/D135180#3835058>, @ldionne wrote:

> In D135180#3834327 <https://reviews.llvm.org/D135180#3834327>, @Mordante wrote:
>
>> In D135180#3834249 <https://reviews.llvm.org/D135180#3834249>, @philnik wrote:
>>
>>> 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?
>>
>> I have a slight preference for the current location. But @ldionne preferred to have them completely at the end.
>> The compiler indeed has ways include optimizations, which we discovered while working on the transitive includes.
>
> Reasons for doing it this way:
>
> 1. Less chance of depending on transitive includes without realizing it
> 2. 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135180