hokein added a comment.
Thanks, the movement looks roughly good.
We should be aware of that (when landing this patch):
- the upstream llvm gn build files need to be adjust (IIUC, there is no
expectation from us to update the gn files)
- we'd better prepare a patch for the internal integration (make the
integration life easier)
================
Comment at: clang-tools-extra/pseudo/CMakeLists.txt:1
+include_directories(include ${CMAKE_CURRENT_BINARY_DIR}/include)
+add_subdirectory(lib)
----------------
Why not use `CMAKE_CURRENT_SOURCE_DIR`? It seems a more natural fit.
================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/DirectiveMap.h:31
+#include "clang-pseudo/Token.h"
#include "clang/Basic/TokenKinds.h"
----------------
`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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D121233/new/
https://reviews.llvm.org/D121233
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
