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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

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.
 if(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR AND NOT APPLE)
----------------
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()
+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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97572