This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Replace `#include ""` with `<>` in libcxx/src/. NFCI
ClosedPublic

Authored by Quuxplusone on Feb 11 2022, 10:04 AM.

Details

Summary

Our best guess is that the two syntaxes should have exactly equivalent
effects, so, let's be consistent with what we do in libcxx/include/.

I've left #include "include/x.h" and #include "../y.h" alone
because I'm less sure that they're interchangeable, and they aren't
inconsistent with libcxx/include/ because libcxx/include/ never
does that kind of thing.

Also, use the _LIBCPP_PUSH_MACROS/POP_MACROS dance for <__undef_macros>,
even though it's technically unnecessary in a standalone .cpp file,
just so we have consistently one way to do it.

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Feb 11 2022, 10:04 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptFeb 11 2022, 10:04 AM
philnik accepted this revision as: philnik.Feb 11 2022, 10:12 AM
philnik added inline comments.
libcxx/src/bind.cpp
9

Not in this PR, but I think it would be a good idea to include the granular header if not all of the header is implemented in one file.

EricWF requested changes to this revision.Feb 11 2022, 11:45 AM
EricWF added a subscriber: EricWF.

Why would we do this?

Our best guess is that the two syntaxes should have exactly equivalent

effects,

They don't. You can use clang++ -E -x c++ - -v < /dev/null to view the search path order for the various include types.

The point of writing includes in the source directory using quotes is to ensure we pick up our local headers, and not something installed along a system path.

This revision now requires changes to proceed.Feb 11 2022, 11:45 AM

Our best guess is that the two syntaxes should have exactly equivalent effects,

They don't. You can use clang++ -E -x c++ - -v < /dev/null to view the search path order for the various include types.

The point of writing includes in the source directory using quotes is to ensure we pick up our local headers, and not something installed along a system path.

Let's suppose for the sake of argument that #include "vector" picks up the correct local libcxx/include/vector while #include <vector> would have picked up the wrong header.
Then, when inside the local libcxx/include/vector itself we see #include <memory> or #include <__utility/forward.h> or whatever, wouldn't that necessarily pick up the wrong header?
How could it be OK to use <> inside the library headers themselves, but not to use <> in src/ when building the library?

There are also enough places we do this by accident already

$ git grep '#include <' ../libcxx/src/ | wc -l
      91

and we're not hearing complaints of breakage from anyone yet.

ldionne accepted this revision as: ldionne.Feb 14 2022, 8:28 AM

I have to agree here, I think this is safe and in fact it's only a source of confusion that we're using "" instead of <>. When we setup the libc++ build, we are careful to suppress the normal C++ stdlib header search paths with nostdinc++, and we add the path to the currently-being-built C++ stdlib explicitly using -I (or -isystem, I forgot).

So this LGTM: I don't think this can break anybody, but I'd like to make sure that @EricWF isn't aware of something we don't know before shipping this.

Rebased on main. @EricWF ping? can you still imagine this breaking things, and if so, how/example plz?

ldionne accepted this revision.Feb 15 2022, 9:04 AM

Because this conflicts with a lot of patches in-flight, let's land this if CI is happy. If there is new information later, we can always make the change in reverse -- but keeping this review open is just going to create churn.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 15 2022, 10:01 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.