[libcxx-commits] [PATCH] D132787: [libc++] Avoids self references in transitive include test.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked an inline comment as done.
Mordante added inline comments.


================
Comment at: libcxx/test/libcxx/transitive_includes.sanitize.py:39
 import re
 import sys
 
----------------
ldionne wrote:
> Instead, perhaps it would be cleaner to figure out the top level header separately (pseudo-code, will need some changes):
> 
> ```
> lines = sys.stdin.readlines()
> 
> get_top_level = lambda line: re.match(r'. .*(?:/|\\\\)include(?:/|\\\\)c\+\+(?:/|\\\\)v[0-9]+(?:/|\\\\)(.+)', line)
> top_level_header = next(get_top_level_header(line) for line in lines if <matches>)
> 
> # Then do the rest exactly as it was
> ...
> 
> # Finally, simply remove the top-level header from the set of all headers
> sorted(set(headers) - set(top_level_header))
> ```
I agree the code can be a bit cleaner and I did a refactoring. I feel your solution looks a bit too clever; reading the finally it seems there is even an issue in this proposal.
`sorted(set(headers) - set(top_level_header))` implies `headers` may contain a `top_level_header`. This should not happen, since this script does a cycle detection.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132787