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

Wed Mar 9 05:31:03 PST 2022

aaron.ballman added a comment.

In D121233#3369736 <https://reviews.llvm.org/D121233#3369736>, @sammccall wrote:

> In D121233#3369657 <https://reviews.llvm.org/D121233#3369657>, @aaron.ballman wrote:
>
>> Thank you for working on this! I have a few thoughts on the renaming, but otherwise strongly support the direction here.
>>
>>> clang/lib/Tooling/Syntax/Pseudo/*           => clang-tools-extra/pseudo/lib/*
>>
>> The usual naming conventions in clang-tools-extra is to use the tool name as the folder it goes in. Based on that, should the folders be `clang-tools-extra/clang-pseudo/` instead of `clang-tools-extra/pseudo/`?
>
> I don't think it's a good naming convention, and am not convinced consistency here is important enough to propagate a bad idea further.

It's the convention we currently use and I don't think we should deviate from it as a one-off; that only adds confusion, especially once others go to add another project and try to use existing projects as a pattern.

> We're in a directory called "clang-tools-extra", so the clang- prefix is redundant in the path (not in the binary name).
> It seems like a small thing, but it adds up: the `clang-tools-extra` directory is ugly, so are `lib` and `include/clang-pseudo`, so is having both `llvm` and `llvm-project` because of the monorepo.
> We have a file named `llvm/llvm-project/llvm/include/llvm/Support/InitLLVM.h`! People working with this code type these paths often.

I don't disagree with these points, FWIW. :-)

> (If it helps, I'd be happy to prepare changes to drop the `clang-` prefixes from any of the directories where they seem obviously redundant - everything except `clang-doc` and `clangd`. But I think the much larger burden of establishing consensus needs to fall on people who think that consistency is worth enforcing here).

The status quo is that we try to be consistent unless there's a compelling reason not to, so I disagree; the onus is on you as to why this is a special snowflake that deserves to be inconsistent with everything else in the project. My suggestion to move forward is: rename to be consistent in this patch so we can land it, then put up an RFC to rename all of the directories in clang-tools-extra at once so that we stay consistent (there's a wee bit extra churn from that, but it means we don't hold up this patch debating directory names). I'd support such an RFC, but I wouldn't support introducing inconsistency here; the rationale for deviation isn't nearly compelling enough to me. Also, because it's an NFC change, I think the RFC is effectively "does someone have a blocking concern that I need to address first, otherwise the NFC change will go in now". It's not so much an RFC as a heads up that the change is coming in case downstreams need to react to it early.

>>> clang/include/clang/Tooling/Syntax/Pseudo/* => clang-tools-extra/pseudo/include/clang-pseudo/*
>>> clang/tools/clang/pseudo/*                  => clang-tools-extra/pseudo/tool/*
>>> clang/test/Syntax/*                         => clang-tools-extra/pseudo/test/*
>>
>> The convention are for clang-tools-extra tests to live in `clang-tools-extra/test/<tool>` (clangd is using the style you propose here when it probably should have followed the existing conventions).
>
> We used this layout for several years, and it was painful to navigate and maintain, so it was deliberately changed in b804eef09052cf40e79aa2ed8a23f2f39e2dda1b at which point all the pain went away.

And introduced new pain for others when you did so -- I went looking for the clangd tests and it took me a while to figure out that project deviated.

> Again, I'd be willing to work on improving this, but I don't want to spend weeks arguing with people about it. My experience is that these conversations are exhausting, negative because people are grumpy about dealing with churn, and in the end there's nobody empowered to say "OK, this is a good idea". See also the idea of using a separate bugtracker, website infra etc for clangd, where all the feedback was extremely negative, it was very difficult to decide to proceed anyway, and these turned out to be large improvements and none of the warnings came true.

Understandable. FWIW, I'd also be fine switching things around so that the test directory is with the tool for all tools (I'd see this as the same kind of NFC change as above). What I'm not fine with is one tool deciding they're special and don't need to be consistent with the rest of the project. These sorts of inconsistencies rarely cause pain for the people who introduce them, but they do cause pain for others who are working on the project as a whole instead of just one tool. As an example of the pain caused by clangd -- not everyone runs tests using a check target. Some folks use python to execute tests directly, and they expect that `python llvm-lit.sv -sv clang-tools-extra\tests` will run all of the tests for clang-tools-extra. I've broken clangd tests by not running the tests when I thought I had. Thankfully, postcommit CI found the issues pretty quickly, but it still meant a bot stampede for me to deal with when I shouldn't have been able to catch the issue far earlier and with less disruption.

So I absolutely sympathize with the points you raise and I'd be happy to support efforts to improve the situation, but I don't think this patch goes about it the right way.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121233