[PATCH] D121233: [pseudo] Move pseudoparser from clang to clang-tools-extra

Tue Mar 15 18:54:43 PDT 2022

sammccall added a comment.

In D121233#3384495 <https://reviews.llvm.org/D121233#3384495>, @thakis wrote:

> Is there a reason why this can't be part of clang-tools-extra/test

This was discussed pretty extensively in the review above.
Other projects across LLVM use proj/{lib,include,unittest,test} and it works well.
clang-tools-extra isn't a project but rather a set of separate projects (few common developers, few changes across them). Mixing the directories of different projects doesn't work well.

I want to make it possible to convert more/all of the directories to separate-project structure, so I'm working on reducing the amount of CMake/lit.cfg boilerplate needed.

> and check-clang-tools?

Yeah, we should add this. The narrower check-clang-pseudo target is essential for development though, and creating merged test targets seems to be a nontrivial amount of CMake goop, so I had put it in the cmake/lit cleanup pile, hoping to get to it soon.

So I can understand better, what is this useful for?
My mental model is: buildbots run check-all, people working on project run check-$project, and check-clang-tools is mostly useful if you made a clang change and want to test its rdeps.

> Why is this in a directory called "pseudo" instead of "clang-pseudo"? This makes everything fairly self-inconsistent (`pseudo/tool` vs `clang-pseudo` in there, but then also `check-clang-pseudo`) – I found this pretty confusing.

It's in a directory with "clang" in the name, the "clang-" prefix only seems necessary in flat namespaces (in `bin/`, cmake targets etc).
Unlike the other directories here, this isn't really a command-line tool, but rather a library. The command-line tool is mostly a demo for lit testing. Maybe `tool/` isn't the right name for that.

(I think it would be useful to drop the clang- prefixes off the other directories here too, but I'm not going to push too hard for that)

> Reading `CMAKE_CURRENT_SOURCE_DIR` in the lit.cfg is fairly unusual. clang's unit tests don't do this, for example.
>
> New lit.cfg input files should use the `path()` abstraction so that relative paths are in the generated lit.cfg output files (which helps for running the tests on a different computer than building them). See a16ba6fea2e554fae465dcaaca1d687d8e83a62e <https://reviews.llvm.org/rGa16ba6fea2e554fae465dcaaca1d687d8e83a62e> for an example.

Thanks. I'd seen that but thought clang-tools-extra didn't have this requirement/wasn't being tested in this way. I'm a bit reluctant to add these without a way to tell if it's doing anything, is there a buildbot to verify this or a script to run?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121233