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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/test/libcxx/diagnostics/detail.headers/algorithm/adjacent_find.module.verify.cpp:1
+
+// -*- C++ -*-
----------------
ldionne wrote:
> 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).
> All your generated headers have this newline at the beginning -- they shouldn't.

Is that really a concern? (I'm going to fix it, but I wanna understand why.)

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

They already do. Evidence: Phab will tell you "no newline before EOF".


================
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:
----------------
Quuxplusone wrote:
> @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.
> @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.

I initially did this (it was waaaay cleaner and could be done by hand too), but modules mode only reports one header error at a time, so we end up with two expected-but-not-seen errors :(

> 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.

It's significant for a future patch that I expect to throw up in a day or so.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105932