aaron.ballman added subscribers: ChuanqiXu, tahonermann, Bigcheese. aaron.ballman added a comment.
In D154130#4481763 <https://reviews.llvm.org/D154130#4481763>, @MrTrillian wrote: > In D154130#4481673 <https://reviews.llvm.org/D154130#4481673>, @aaron.ballman > wrote: > >> Adding a few more folks who are interested in lit changes to try to get the >> review unstuck. >> >> FWIW, I worry about the subtlety of the `>` change because it's not entirely >> clear to me when I'd need to use `%>t` in a test. I worry code reviewers >> will miss this sort of thing and we'll only find out there's an issue when >> the test fails for someone with a problematic path. Is there a rule of thumb >> we should be following for its use? > > Thanks for the extra reviewers! > > 95% of the `%>t` are around clang modulemap files, because that code resolves > real paths in C++ by design, so I can't avoid it. In fact I should rename > `PREFIX_EXPANDED` to `MODULEMAP_PREFIX` so it would be much clearer. Okay, if this is mostly specific to module maps, that may resolve most of my concern (we don't add a lot of new tests there, so it's less of a burden for reviewers). CC @ChuanqiXu @Bigcheese @tahonermann to see if there are modules concerns with this. > There are three cases where I didn't expect to need the expanded paths: > `relative_include.m`, `case-insensitive-include-win.c` and > `module-header-mismatches.m`. There may be a way to change the clang > implementation so it does not need to have expanded paths, but that felt like > a different investigation. > > I'm happy to consider alternative syntaxes to `%>t` too. I think the syntax is reasonable enough (at least, I don't have arguably better suggestions), it was more just the "how will I know when to use it?" concerns. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154130/new/ https://reviews.llvm.org/D154130 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
