[clang-tools-extra] [llvm] [llvm] add support for mustache templating language (PR #105893)
Paul Kirth via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 9 10:55:02 PDT 2024
ilovepi wrote:
U
> I like Mustache! Just took a quick look at this following your Discourse post.
>
> Do the custom allocators make any significant performance difference (measure)? Wouldn't want to prematurely optimize. They're the kind of thing that make maintenance annoying and can be hard to remove later.
>
Using the arena allocator simplifies the code greatly. We don't need RAII, and the lifetimes of these allocations are well scoped to the lifetime of the arena. It's also the pattern we use elsewhere in the project for parsing/tokenizing, which is why I suggested the pattern in the first place.
> Same with `SmallString<0>` / `SmallVector`, in some places `std::string` may be sufficient. I think this first implementation can afford to be as simple as possible, specific implementation optimizations can be added later.
>
Those are already part of the advice in the programmer's manual. Using them also allows us to benefit from additional testing via expensive checks/reverse iteration, etc.
> Should `Token` and `ASTNode` be present in the `Mustache.h` header at all?
That's a fair point. For the API surface we're exposing, it may be better to make those private to the implementation, or at least not part of the public header. To some extend that's dependent on how the library will be used in practice, though. @PeterChou1 what are your thoughts here?
https://github.com/llvm/llvm-project/pull/105893
More information about the llvm-commits
mailing list