sammccall added a comment. Only serious concern is `getPreviousToken()`.
================ Comment at: clang/lib/Format/FormatTokenSource.h:74 public: - IndexedTokenSource(ArrayRef<FormatToken *> Tokens) + IndexedTokenSource(SmallVectorImpl<FormatToken *> &Tokens) : Tokens(Tokens), Position(-1) {} ---------------- 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) ================ Comment at: clang/lib/Format/FormatTokenSource.h:94 FormatToken *getPreviousToken() override { return Position > 0 ? Tokens[Position - 1] : nullptr; } ---------------- 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) ================ Comment at: clang/lib/Format/FormatTokenSource.h:115 unsigned getPosition() override { LLVM_DEBUG(llvm::dbgs() << "Getting Position: " << Position << "\n"); ---------------- 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) ================ Comment at: clang/lib/Format/FormatTokenSource.h:130 + assert((*New.rbegin())->Tok.is(tok::eof)); + LLVM_DEBUG(llvm::dbgs() << "Inserting:\n"); + int Next = Tokens.size(); ---------------- nit: move into the LLVM_DEBUG block below? or are you worried about a crash? ================ Comment at: clang/lib/Format/FormatTokenSource.h:151 private: + int next(int Current) { + int Next = Current + 1; ---------------- nit: const I wasn't sure if this method advanced or not. "successor" might be clearer in this respect ================ Comment at: clang/lib/Format/FormatTokenSource.h:173 + // stream continues at position b instead. + std::map<int, int> Jumps; }; ---------------- DenseMap? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143070/new/ https://reviews.llvm.org/D143070 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits