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

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/pseudo/CMakeLists.txt:1
+include_directories(include ${CMAKE_CURRENT_BINARY_DIR}/include)
+add_subdirectory(lib)
----------------
sammccall wrote:
> hokein wrote:
> > Why not use `CMAKE_CURRENT_SOURCE_DIR`? It seems a more natural fit.
> I don't understand what you want me to change.
> This line adds the "include" directory, both in the source tree and the genfiles tree.
> 
> Do you want the former to be explicitly qualified with `CMAKE_CURRENT_SOURCE_DIR` rather than using the relative path? That seems unnecessary and uncommon 
> 
> If it's not obvious what this line does, would it be better to split it into two calls with one arg each?
sorry, I was not clear in the comment.

> If it's not obvious what this line does, would it be better to split it into two calls with one arg each?

Yeah. I interpreted the line `include_directories(include ${CMAKE_CURRENT_BINARY_DIR}/include)` as "add the `${CMAKE_CURRENT_BINARY_DIR}/include` to the include-search directory", and didn't realize it covers the `<source_dir>/include`.

Split it into two or add a comment would make it clearer.


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/DirectiveMap.h:31
 
+#include "clang-pseudo/Token.h"
 #include "clang/Basic/TokenKinds.h"
----------------
sammccall wrote:
> hokein wrote:
> > `clang-pseudo` is somewhat nice, it is probably ok, but 
> > - using hyphen in the include path is not a canonical pattern in LLVM code-style;
> > - we have a command-line tool which calls `clang-pseudo`, clang-pseudo makes me think this is a header from the command-line tool rather from the *library*.
> > 
> > there are other options, but they don't seem significantly better than the current one :(
> > 
> > - `clang/Pseudo/Token.h`: this matches the canonical pattern of using headers from clang libraries, but it causes confusion (which we want to avoid in this patch);
> > - `clangPseudo/Token.h`: this feels slightly better, as we build a library called `clangPseudo`;
> > 
> > 
> > I know we want to explicitly express that this is intended to be a library, another aggressive option (which mostly follows the existing pattern in clang-tools-extra) would be
> > 
> > ```
> > clang-tools-extra/clangPseudo/Token.{h, cpp}
> > clang-tools-extra/clangPseudo/internal/Forest.h
> > clang-tools-extra/clangPseudo/tool/
> > ```
> > 
> > internal private headers are put into the sub `internal` directory, headers in non `internal` directories are treat as public headers implicitly.
> Thanks for going through the alternatives.
> I think `clang-pseudo` is the best option, and `clangPseudo` is the most viable alternative.
> `clangPseudo` looks wrong to me though, and I think `clang-pseudo` follows precedent better, both inside LLVM and more generally.
> 
> > using hyphen in the include path is not a canonical pattern in LLVM code-style;
> 
> Let's split include path into parts: `#include "mount-point/ProjectRelativeSubdir/Header.h"`
> 
> Mount points are usually single-word lowercase in LLVM.
> The only real precedent for multi-words is `clang-c`/`llvm-c`/`mlir-c` that use hyphens. This isn't terribly strong, but it's something.
> 
> Project-relative subdirs follow the internal structure of the project, which is usually UpperCamelCase and occasionally lower-hyphen-case.
> 
> AFAICS there are zero examples of `lowerCamelCase` either in mount points or in project-relative subdirs.
> (I believe this is rare in libraries outside llvm codebase too, which may be why it looks wrong to me).
> 
> > clangPseudo/Token.h: this feels slightly better, as we build a library called clangPseudo;
> 
> Yes, matching the library name rather than the binary name feels more consistent.
> I don't think what CMake calls the library is a particularly significant string to anchor on (particularly for out-of-tree users, but even for llvm devs). Confusion with the `clang-pseudo` tool is a risk, on the other hand *association* with the tool is a good thing.
> 
> The obvious precedent here is that `clang` is both a binary and an include mount point. I don't think this has caused confusion.
> 
> > clang/Pseudo/Token.h: this matches the canonical pattern of using headers from clang libraries, but it causes confusion (which we want to avoid in this patch)
> 
> Agree, this isn't a good idea if we want to send a signal that this is a sibling to clang, but not part of it.
> 
> > clang-tools-extra/clangPseudo/Token.{h, cpp}
> 
> I did consider this option, and very much wanted it to work. I think it's a bad idea though:
> 
> It places public headers next to the implementation and internal headers elsewhere, which is the **opposite** of the convention in other libraries.
> It can easily lead to problems: the distinction between public vs private interface is subtle and inadvertently making too many headers public won't break the build and is easily overlooked in review.
That's fair enough. I'm convinced clang-pseudo is the best option, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121233