[PATCH] D143070: [clang-format] Enable FormatTokenSource to insert tokens.
Manuel Klimek via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Feb 14 12:57:13 PST 2023
klimek added inline comments.
================
Comment at: clang/lib/Format/FormatTokenSource.h:74
public:
- IndexedTokenSource(ArrayRef<FormatToken *> Tokens)
+ IndexedTokenSource(SmallVectorImpl<FormatToken *> &Tokens)
: Tokens(Tokens), Position(-1) {}
----------------
sammccall wrote:
> As I understand it, this parameter is used:
> - to provide the initial set of input tokens the source will iterate over
> - as a common address space for input + synthesized tokens, to allow the jump mechanism to work
> - to make the caller responsible for ownership/lifetime of the synthesized tokens too
>
> This simplifies the implementation, my only problem with this is it seems unusual and confusing.
> A comment explaining the roles of this `Tokens` param would help a bit.
>
> Alternatively you could consider slightly different data structures just for the purpose of making interfaces more obvious: e.g. pass in a `BumpPtrAllocator&`, allocate scratch tokens there, and use pointers instead of indices (jumps become a ptr->ptr map etc)
Noticing none of this makes sense. We should really just copy the tokens, given that we modify the vector.
================
Comment at: clang/lib/Format/FormatTokenSource.h:94
FormatToken *getPreviousToken() override {
return Position > 0 ? Tokens[Position - 1] : nullptr;
}
----------------
sammccall wrote:
> this no longer seems valid, immediately after calling insertTokens(), Position is at the start of a jump range, and Tokens[Position - 1] will be the EOF at the end of the main stream or previous jump range.
>
> if this is never going to happen, you can detect it (I don't think Tokens[Position - 1] can be EOF in any other way)
Added comment in the interface and assert.
================
Comment at: clang/lib/Format/FormatTokenSource.h:115
unsigned getPosition() override {
LLVM_DEBUG(llvm::dbgs() << "Getting Position: " << Position << "\n");
----------------
sammccall wrote:
> maybe add a comment that positions don't have meaningful order and this is only useful to restore the position?
>
> (All the callsites look good, it just feels like a trap)
Added a comment.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D143070/new/
https://reviews.llvm.org/D143070
More information about the cfe-commits
mailing list