[libcxx-commits] [PATCH] D105932: [libcxx][modules] protects users from relying on libc++ detail headers (1/n)

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Good idea about adding tests, thanks for doing that.

It does raise a couple issues that I don't think we've had before:

1. We'll now have to re-generate the auto-gen tests very often, since we add detail headers a lot more often than we add public headers. I don't think that's an actual issue, especially since we now have the `libcxx-generate-files` target to do it, but it's worth keeping in mind. If it becomes a pain, we could potentially go for a different approach where the tests are generated automatically on the fly when we run the test suite. Unless that becomes a real issue, I do have a preference for something simple where we can see the generated sources directly, like today.

2. Today, we never have to delete auto-gen tests, because we basically never remove/rename public headers. This commit changes that. Since there is some churn in our private headers (which is okay), we might start accumulating older headers that have since then been removed/renamed. I think a simple way to solve that would be to remove the whole `libcxx/test/libcxx/diagnostics/detail.headers` directory in your generation script. Also, if someone manually modifies one of the header tests (or tries adding their own), CI would fail in the `check-generated-output` CI job (since whatever they added would be removed).



================
Comment at: libcxx/test/libcxx/diagnostics/detail.headers/algorithm/adjacent_find.module.verify.cpp:1
+
+// -*- C++ -*-
----------------
All your generated headers have this newline at the beginning -- they shouldn't.

I also think they should have a newline at the end (might not be required technically but we always do that).


================
Comment at: libcxx/test/libcxx/diagnostics/detail.headers/algorithm/adjacent_find.module.verify.cpp:13
+
+// This test was generated by `libcxx/utils/generate_private_header_tests.py`.
+
----------------
This could look more like:

```
// WARNING: This test was generated by {script_name}
// and should not be edited manually.
```

That's what we do in other generated tests.


================
Comment at: libcxx/utils/generate_private_header_tests.py:54
+        test_directory = path_with_subdir.group(
+            1) + '/' if path_with_subdir else ""
+        test_name = path_with_subdir.group(2) if path_with_subdir else path[2:]
----------------
Awkward line break.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105932