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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
