[libcxx-commits] [PATCH] D99309: [libc++] Header inclusion tests

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

Quuxplusone marked an inline comment as done and 2 inline comments as not done.
Quuxplusone added inline comments.


================
Comment at: libcxx/include/chrono:16
 
+#include <compare>
+
----------------
cjdb wrote:
> Have we added `<compare>` support for `<chrono>` yet? If not, I'd prefer to keep it out of the synopsis. (If you're keeping it in, please add `// C++20` or something.)
There's no particular "support" needed; I'm just making libc++ provide the header inclusion that is mandated by the Standard.

...On second glance, I guess you're objecting //only// to the comment on line 16, and not to the functional conformance improvement on line 830. Is that right?  I have no strong objection to removing line 16 (and likewise throughout the comments), and am inclined to just do it. It'd shrink the diff, too!
Anyone else care to advocate for //keeping// these new synopsis comments?


================
Comment at: libcxx/test/libcxx/inclusions/algorithm.inclusions.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
cjdb wrote:
> Can these please be placed under `libcxx/test/std/.../${subclause}/${subclause}.syn/` instead? They're responsible for checking the synopsis, so I'd prefer they sit with the other tests of their category.
Unfortunately there's no way to do that, without some human coming up with the ad-hoc list of subclauses and creating all those directories. And then we'd have the autogenerated tests scattered all over the codebase, which I foresee leading to problems as people add and remove autogenerated tests. So, no.


================
Comment at: libcxx/utils/generate_header_inclusion_tests.py:171
+//
+// clang-format off
+{markup}
----------------
Mordante wrote:
> Wouldn't it be nicer when the script produces properly formatted code?
Yes-ish, but my impression is we can't convince clang-format to accept this code.
Anyway, I just copy-pasted this line from the other two generator scripts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99309