[PATCH] D129648: Use pseudo parser for folding ranges

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:175
+
+  llvm::Optional<pseudo::TokenStream> Preprocessed;
+  Preprocessed = DirectiveStructure.stripDirectives(OrigStream);
----------------
nit: no need to use llvm::Optional.


================
Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:178
+
+  auto ParseableStream = clang::pseudo::stripComments(
+      cook(*Preprocessed, clang::pseudo::genericLangOpts()));
----------------
Do we want to strip comments? I think we can keep the comments.


================
Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:185
+    if (auto *Paired = Tok.pair()) {
+      if (Tok.Line < Paired->Line) {
+        Position Start = offsetToPosition(
----------------
The `if` logic seems tricky, it is doing two different things:

1) avoid creating duplicate `FoldingRange` for a pair bracket
2) avoid creating a FoldingRange if the pair bracket is on the same line.

Are both intended?


================
Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:187
+        Position Start = offsetToPosition(
+            Code, OrigStream.origToken(Tok).text().data() - Code.data());
+        Position End = offsetToPosition(
----------------
the ` OrigStream.origToken` syntax seems weird from the caller side (it is counter-intuitive).

I think  `OrigStream.tokens()[T.OrigTokIdx]` is clearer.  


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/Token.h:72
+  /// Index into the original token stream (of the original source file).
+  Index OrigTokIdx = 0;
   // Helpers to get/set Flags based on `enum class`.
----------------
I'd suggest initializing it to `Index::Invalid`.

it'd be nice to add some tests in the `pseudo/unittests/TokenTest.cpp`.




================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/Token.h:182
+  /// Extracts the token in original stream corresponding to Token T.
+  const Token &origToken(const Token &T) const {
+    assert(isOriginal() && "stream is derived");
----------------
I'm not sure we should have this API, it seems error-prone to use, and it only makes sense for the original token stream.  (I'd just remove it, and use `OrigStream.tokens()[T.OrigTokIdx]` directly, see my other comment)



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129648/new/

https://reviews.llvm.org/D129648