[PATCH] D143070: [clang-format] Enable FormatTokenSource to insert tokens.

[Sam McCall via Phabricator via cfe-commits]

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