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

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

Quuxplusone added inline comments.


================
Comment at: libcxx/utils/generate_private_header_tests.py:29-32
+    if not os.getcwd().endswith("libcxx"):
+        print(
+            "generate_private_header_tests.py: This script must be run in exactly 'llvm-project/libcxx'."
+        )
----------------
Please follow the magic incantation at the top of all the other test-generating scripts, so that this script can be run from anywhere.


================
Comment at: libcxx/utils/generate_private_header_tests.py:56
+        test_name = path_with_subdir.group(2) if path_with_subdir else path[2:]
+        path_to_write = f'test/libcxx/diagnostics/detail.headers/{test_directory}{test_name}.module.verify.cpp'
+        with open(path_to_write, 'w') as f:
----------------
@ldionne wrote:
> 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.

Another idea (and maybe a better one for the maintainers' sanity) would be to generate just one test file per //public// header. Instead of generating `detail.headers/utility/{__decay_copy,as_const,cmp,declval,...}.module.verify.cpp`, simply generate //one// test file, `detail.headers/utility.verify.cpp`, containing the code
```
// expected-error@*:* {{use of private header from outside its module: '__utility/__decay_copy.h'}}
#include <__utility/__decay_copy.h>

// expected-error@*:* {{use of private header from outside its module: '__utility/as_const.h'}}
#include <__utility/as_const.h>

// expected-error@*:* {{use of private header from outside its module: '__utility/cmp.h'}}
#include <__utility/cmp.h>
```
and so on and so on. Then these files would change often, but be added/deleted rarely, just like all the other generated test files.

Finally: Is the `.module.` in `foo.module.verify.cpp` significant to llvm-lit? Or could we go with just `foo.verify.cpp`, for clarity? `find ../libcxx/test/ -name \*.module.\*` doesn't show any results in trunk at the moment, so I suspect it's not significant and can be removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105932