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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
