Page MenuHomePhabricator

[NFC][libc++] Moves transitive includes location.
ClosedPublic

Authored by Mordante on Oct 4 2022, 10:35 AM.

Details

Reviewers
ldionne
philnik
Group Reviewers
Restricted Project
Commits
rG040f50ad6bad: [NFC][libc++] Moves transitive includes location.
Summary

This moves some includes missed in D133212.

Diff Detail

Event Timeline

Mordante created this revision.Oct 4 2022, 10:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 4 2022, 10:35 AM
Mordante requested review of this revision.Oct 4 2022, 10:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 4 2022, 10:35 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision.Oct 4 2022, 10:36 AM
This revision is now accepted and ready to land.Oct 4 2022, 10:36 AM
philnik requested changes to this revision.Oct 4 2022, 10:41 AM
philnik added a subscriber: philnik.

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?

This revision now requires changes to proceed.Oct 4 2022, 10:41 AM

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.

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.

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.

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.

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.

Mordante updated this revision to Diff 467168.Oct 12 2022, 8:48 AM

Rebased and addresses review comments.

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.

Thanks for catching this @philnik. I've discussed this with @ldionne yesterday and I only left the moving of the transitive included to the end of the file inside the include guards.

Mordante edited the summary of this revision. (Show Details)Oct 12 2022, 10:16 AM
philnik accepted this revision.Oct 12 2022, 10:36 AM

LGTM with green CI.

This revision is now accepted and ready to land.Oct 12 2022, 10:36 AM
This revision was automatically updated to reflect the committed changes.