[libcxx-commits] [PATCH] D116958: [libc++] Alphabetize CMakeLists.txt and module.modulemap; enforce in CI.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

For each of these tests, can you please add a short comment saying something like `# This test checks that include files in the CMakeLists.txt are sorted alphabetically`, and similarly for the other ones.



================
Comment at: libcxx/test/libcxx/lint/lint_cmakelists.sh.py:6
+
+if __name__ == '__main__':
+    libcxx_test_libcxx_lint = os.path.dirname(os.path.abspath(__file__))
----------------
Quuxplusone wrote:
> ldionne wrote:
> > Is this really useful? IMO it just has the effect of indenting everything artificially.
> No more than `int main() {` in C++. :) In normal Python programs, this idiom prevents the script from auto-running and having crazy side effects when you `import lint_cmakelists` (not that anyone //should// ever do that). Anyway, it's consistent with `libcxx/utils/*.py`, and with my prejudices, so I propose to keep it.
Yeah, I'm aware of the convention for normal Python scripts, but I don't see the point here.

I'll make my vote neutral, so keep it if you want.


================
Comment at: libcxx/test/libcxx/lint/lit.local.cfg:3
+import pipes, sys
+config.substitutions.append(('%{python}', pipes.quote(sys.executable)))
----------------
Can you try adding `print(config.substitutions)` here and checking whether `%{libcxx}` is in there somewhere? It should.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116958