[PATCH] D129648: Use pseudo parser for folding ranges

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/CMakeLists.txt:149
+target_include_directories(clangDaemon PUBLIC
+  ${CMAKE_CURRENT_SOURCE_DIR}/../pseudo/include
+)
----------------
It should rather be up to the clangPseudo target to specify its include directories, if it doesn't already

I think interface_include_directories() is the right function (though knowing llvm maybe it's wrapped somehow)

https://cmake.org/cmake/help/latest/prop_tgt/INTERFACE_INCLUDE_DIRECTORIES.html


================
Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:158
 
 // FIXME(kirillbobyrev): Collect comments, PP conditional regions, includes and
 // other code regions (e.g. public/private/protected sections of classes,
----------------
we should transfer this fixme, either now or when we remove this code


================
Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:176
+  llvm::Optional<pseudo::TokenStream> Preprocessed;
+  Preprocessed = DirectiveStructure.stripDirectives(OrigStream);
+
----------------
This means we won't offer folding ranges in disabled-PP regions.
This is a significant regression of the client-side fallback!
It's not trivial to address though. Add a FIXME?


================
Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:178
+
+  auto ParseableStream = clang::pseudo::stripComments(
+      cook(*Preprocessed, clang::pseudo::genericLangOpts()));
----------------
hokein wrote:
> Do we want to strip comments? I think we can keep the comments.
Stripping comments doesn't seem necessary to me, maybe I'm missing something.


================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:272
     auto T = Annotations(Test);
-    auto AST = TestTU::withCode(T.code()).build();
-    EXPECT_THAT(gatherFoldingRanges(llvm::cantFail(getFoldingRanges(AST))),
-                UnorderedElementsAreArray(T.ranges()))
+    EXPECT_THAT(
+        gatherFoldingRanges(llvm::cantFail(getFoldingRanges(T.code().str()))),
----------------
You've removed the tests for the old implementation, but it's still used by ClangdServer.

Test both versions instead?


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/Token.h:71
   uint8_t Flags = 0;
+  /// Index into the original token stream (of the original source file).
+  Index OrigTokIdx = 0;
----------------
nit: not clear what "original source file" is opposed to

maybe (as raw-lexed from the source code)


================
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`.
----------------
hokein wrote:
> I'd suggest initializing it to `Index::Invalid`.
> 
> it'd be nice to add some tests in the `pseudo/unittests/TokenTest.cpp`.
> 
> 
OriginalIndex (avoid cryptic abbreviations, "token" is implied)


================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/Token.h:176
 
+  /// An original token stream corresponds to the original source file (Eg.
+  /// produced by lex()). It is not derived from another token stream.
----------------
I don't like the stream knowing about this attribute, there's no reasonable way to actually use it dynamically at runtime, we're complicating the API just so we can add some assertions.

What's wrong with OrigToken = OrigStream.tokens()[T.OrigIndex]?

(If we're going to teach the stream about the "derived stream" concept, it seems more natural to give it a pointer to its parent and allow mapping to any ancestor stream)


================
Comment at: clang-tools-extra/pseudo/lib/Lex.cpp:30
+  // Index into the token stream of original source code.
+  unsigned TokenIdx = 0;
   unsigned LastOffset = 0;
----------------
nit: unsigned -> Token::Index
Idx -> Index? (it's not too cryptic here but also only saving 2 chars)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129648