sammccall added inline comments.
================
Comment at: clang-tools-extra/pseudo/CMakeLists.txt:1
+include_directories(include ${CMAKE_CURRENT_BINARY_DIR}/include)
+add_subdirectory(lib)
----------------
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?
================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/DirectiveMap.h:31
+#include "clang-pseudo/Token.h"
#include "clang/Basic/TokenKinds.h"
----------------
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.
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
