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

Wed Mar 9 11:47:24 PST 2022

sammccall added a comment.

In D121233#3370177 <https://reviews.llvm.org/D121233#3370177>, @aaron.ballman wrote:

> I agreed based on the understanding that the new code would follow the existing conventions, so perhaps the agreement was a bit premature. But, I'm honestly pretty surprised at this significant of resistance to following the same patterns used by (basically) everything else in the project.

I'm not sure I buy the premise ("everything else in the project"). clang-tools-extra isn't really a project: it doesn't have coherent code, goals, or developers.
Each subproject in llvm-project **except** those under clang-tools-extra has a root directory containing tests etc for the subproject, and makes good use of this!

> We're talking about a directory name and where we put tests; what are the technical challenges here? All of the arguments you've presented seem to be stylistic in nature (not that those kinds of issues aren't important to consider), so I feel like I must be missing something.

Right, directory name is a stylistic concern either way AFAICT (redundancy and consistency).

Location of tests is more complicated, and more important. Experience from clangd:

- A root directory for build system config is important. IIRC the straw that broke the camel's back for rearranging clangd was having a root CMakeLists file to turn it off in one obvious place.
- The most common development tools (editors, shells, grep, ...) are much harder to use if your working set is several directories rather than one.
- Linking strangers to the code <https://github.com/llvm/llvm-project/tree/main/clang-tools-extra/clangd> leaves the tests essentially invisible.
- It clearly defines the boundaries of the project. Before the move, I don't think most contributors could have answered the question "which directories are part of clangd" with confidence. (I couldn't!)
- On multiple occasions contributors failed to find one of (unit|lit) tests, because once you're looking in nonobvious places you stop when you find anything. So they put tests in the wrong place.
- The clang-tools-extra layout doesn't provide test targets for subprojects, which is a dealbreaker; it seems people resort to guessing at llvm-lit invocations instead. We were maintaining `check-clangd` in clang-tools-extra/test/CMakeLists.txt as a special snowflake, which is presumably also objectionable (it's 35 lines of CMake).

>> If consistency is to be the sine qua non, then I think we should probably leave the code where it is until someone's willing to do the work you described. clang/Tooling/Syntax is the most consistent location for the code, and the one proposed in the original RFC last year.
>
> If you don't feel like updating the projects to do a consistent rename, that's totally fine; I didn't intend to imply that was expected of you or required for this to proceed.

When you say it's fine, do you mean that it's acceptable to land this patch, or to abandon it, or to land your preferred version that I'm not OK with? :-)

I think we are now is:

- there was an RFC
- several months later, code started landing, and there was new discussion (on the RFC thread) including a proposal to move
- we agreed that a move is OK with us, but are stuck on the details

So if it makes a difference, it seems you're _requesting_ a change here, not approving one.

> The directory layout and naming structure has worked well enough thus far

If people finding build/test setup weird and alienating, I think they're more likely to not contribute than to offer clear feedback. I'm not sure how we can tell it works well.
Anecdotally, I do know that I was put off from contributing to clang-tidy by how hard it was to maintain and debug tests, I've lost significant time when I've needed to do this, and I've seen others struggle with it too.
But while this has come up many times in conversation, I've never seen it written down. Does open-source have a taboo about whining about stuff being hard? :-)

> I think part of the issue here is that clang-tools-extra is a dumping ground for projects, so it isn't itself a "project" (in the same sense of LLVM, Clang, lldb, etc).

I do agree with this perspective, clang-tools-extra is "just a directory".
This is why it didn't seems like we were doing something wild with clangd: we were just treating it like a "real" subproject under the llvm-project umbrella.
(I'd be happy with moving clangd or clang-pseudo directly under llvm-project if that addresses the concerns, as long as I don't have to chase impossible consensus-via-RFC).

> I was originally concerned about the pseudo parser existing in the tree at all because of maintenance burdens, but those concerns were mitigated by moving the project here to clang-tools-extra. I don't think the directory name being `pseudo` instead of `clang-pseudo` will add significant maintenance burden; it'll just be the only project we can't figure out the executable name

The executable we're talking about here is essentially a driver for tests, so this is similar to not being able to find that the name of `clang/Index` or `tools/libclang` is `c-index-test`.

> But the test directory being in a novel location really does add maintenance burden in the form of making it trivially easy to think you've run tests when you haven't actually done so.

I'm sorry that we broke this workflow, churn is painful. I think it got fixed though!
It's also a fragile workflow which doesn't update dependencies, which is why we don't use it and hadn't tested it.
Are you sure this isn't a symptom of relying on low-level tools because `check-clang-tools` is too coarse-grained?

> This is has already caused me pain with clangd and I don't want to see that being used as precedent to make things worse here.

This feels like a double standard - the precedents you've cited have already caused me pain, too.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121233