[libcxx-commits] [PATCH] D97572: [libc++] Include <__config_site> from <__config>

Arthur O'Dwyer via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Wed Mar 10 13:31:38 PST 2021

Quuxplusone added a comment.

Minor drive-by comments. I'm extremely unlikely to voice either approval or disapproval in an official capacity. :)

Comment at: libcxx/CMakeLists.txt:407
+# TODO: Other projects rely on the location where we generate the libc++ headers
+#       internally. We should get rid of these dependencies.
This comment is scary but uninformative. What "other projects" do you mean? By "these dependencies" do you mean the dependency //from// those other projects //to// the specific string "c++/v1" (in which case surely you should say `They` not `We`)? or by "these dependencies" do you mean one or more of lines 408-424?
Do you mean `They should use LIBCXX_GENERATED_INCLUDE_DIR instead of hard-coding include/c++/v1`?

Comment at: libcxx/docs/TestingLibcxx.rst:44
+before running the tests manually with `lit` when you make any sort of change,
+including to the headers.
Is running `[ninja|make] cxx` specifically //not// enough? (I'm guessing the answer is "yes, not enough.") That answer should be called out explicitly in the text.

Comment at: libcxxabi/CMakeLists.txt:173
+    target_compile_options(cxx-headers INTERFACE -I "${LIBCXXABI_LIBCXX_INCLUDES}")
+  endif()
Lines 170 and 172 are identical except for `/I` versus `-I`. I know Microsoft's command-line compiler driver supports `-I` as a synonym for `/I`. So could we combine these five lines into one line using `-I` in both cases? Or is the slash character required because it's magic to CMake, or something like that?

  rG LLVM Github Monorepo



More information about the libcxx-commits mailing list