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

Wed Mar 9 08:37:46 PST 2022

aaron.ballman added a comment.

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

> I understand where you're coming from. But I think agreeing to move the code was premature if it means either:
>
> - following all the precedents in clang-tools-extra, or
> - taking on the political burden of getting these changed for all projects.

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. 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.

> Based on previous interactions, I think we differ on the relative value we place on consistency vs local quality vs burden on development, and our ability to make consensus-based changes on a timeline that feels acceptable.
> (This isn't a criticism, I admire your patience and dedication to consistency, I just don't share it).
> 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.

>> 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.
>
> If I understand, you're saying that LLVM in general, or clang-tools-extra in particular values consistency over quality.

You've not convinced me there's a quality issue here, just a difference in stylistic preference. The directory layout and naming structure has worked well enough thus far (excepting perhaps the test directory for clangd, but I'll be frank: I don't get why that change was needed and it's made my life worse since it was made, so I regret not paying closer attention to that review).

If there's a quality concern here that I'm not yet seeing, I'd like to know more about it. I definitely don't want to prefer consistency over quality.

> Is this a documented policy (where?), consensus (among who?), or historical practice (which would beg the question somewhat).
> This is a genuine question - I suspect that you're right, but it's hard to know how to challenge this if it's unclear where it comes from.

Not clearly documented anywhere (but sort of is documented; it's how we handle everything else in the coding style guidelines; stick with the conventions used locally as best you can). I would call it a mixture of historical practice and general common practice in the industry. Consistency in interfaces is generally easier on newcomers to a project because it reduces the amount of mental burden to understand the project's architecture. 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 think that adds some natural tension here because the needs for a random group of projects is hard to get consistency out of as we add more and more projects. It's probably good that we're having this discussion, but I'd prefer if the finer points on policy didn't hold up your patch either.

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 of from the base directory name (across the entire monorepo, perhaps? I can't find other examples with a quick look). That's.. silly, but not the end of the world or worth holding this patch up over if you insist on not having the `clang-` prefix that the executable has. 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. 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.

Can you help me understand the quality concerns you mentioned?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121233