dgoldman updated this revision to Diff 358606.
dgoldman added a comment.
Fix clang tidy warnings
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D105904/new/
https://reviews.llvm.org/D105904
Files:
clang-tools-extra/clangd/CMakeLists.txt
clang-tools-extra/clangd/FindSymbols.cpp
clang-tools-extra/clangd/ParsedAST.cpp
clang-tools-extra/clangd/ParsedAST.h
clang-tools-extra/clangd/Protocol.h
clang-tools-extra/clangd/TextMarks.cpp
clang-tools-extra/clangd/TextMarks.h
clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
clang/include/clang/Lex/PPCallbacks.h
Index: clang/include/clang/Lex/PPCallbacks.h
===================================================================
--- clang/include/clang/Lex/PPCallbacks.h
+++ clang/include/clang/Lex/PPCallbacks.h
@@ -492,6 +492,11 @@
Second->PragmaComment(Loc, Kind, Str);
}
+ void PragmaMark(SourceLocation Loc, StringRef Trivia) override {
+ First->PragmaMark(Loc, Trivia);
+ Second->PragmaMark(Loc, Trivia);
+ }
+
void PragmaDetectMismatch(SourceLocation Loc, StringRef Name,
StringRef Value) override {
First->PragmaDetectMismatch(Loc, Name, Value);
Index: clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
+++ clang-tools-extra/clangd/unittests/FindSymbolsTests.cpp
@@ -1027,6 +1027,106 @@
AllOf(WithName("-pur"), WithKind(SymbolKind::Method))))));
}
+TEST(DocumentSymbolsTest, PragmaMarkGroups) {
+ TestTU TU;
+ TU.ExtraArgs = {"-xobjective-c++", "-Wno-objc-root-class"};
+ Annotations Main(R"cpp(
+ $DogDef[[@interface Dog
+ @end]]
+
+ $DogImpl[[@implementation Dog
+
+ + (id)sharedDoggo { return 0; }
+
+ #pragma $Overrides[[mark - Overrides
+
+ - (id)init {
+ return self;
+ }
+ - (void)bark {}]]
+
+ #pragma $Specifics[[mark - Dog Specifics
+
+ - (int)isAGoodBoy {
+ return 1;
+ }]]
+ @]]end // FIXME: Why doesn't this include the 'end'?
+
+ #pragma $End[[mark - End
+]]
+ )cpp");
+ TU.Code = Main.code().str();
+ EXPECT_THAT(
+ getSymbols(TU.build()),
+ ElementsAre(
+ AllOf(WithName("Dog"), SymRange(Main.range("DogDef"))),
+ AllOf(WithName("Dog"), SymRange(Main.range("DogImpl")),
+ Children(AllOf(WithName("+sharedDoggo"),
+ WithKind(SymbolKind::Method)),
+ AllOf(WithName("Overrides"),
+ SymRange(Main.range("Overrides")),
+ Children(AllOf(WithName("-init"),
+ WithKind(SymbolKind::Method)),
+ AllOf(WithName("-bark"),
+ WithKind(SymbolKind::Method)))),
+ AllOf(WithName("Dog Specifics"),
+ SymRange(Main.range("Specifics")),
+ Children(AllOf(WithName("-isAGoodBoy"),
+ WithKind(SymbolKind::Method)))))),
+ AllOf(WithName("End"), SymRange(Main.range("End")))));
+}
+
+TEST(DocumentSymbolsTest, PragmaMarkGroupsNoNesting) {
+ TestTU TU;
+ TU.ExtraArgs = {"-xobjective-c++", "-Wno-objc-root-class"};
+ Annotations Main(R"cpp(
+ // FIXME: We miss the mark below, is it in the preamble instead?
+ // Unclear if it's worth supporting, likely very uncommon.
+ #pragma mark Helpers
+ void helpA(id obj) {}
+
+ #pragma mark -
+ #pragma mark Core
+
+ void coreMethod() {}
+ )cpp");
+ TU.Code = Main.code().str();
+ EXPECT_THAT(
+ getSymbols(TU.build()),
+ ElementsAre(AllOf(WithName("helpA")), AllOf(WithName("(unnamed group)")),
+ AllOf(WithName("Core")), AllOf(WithName("coreMethod"))));
+}
+
+TEST(DocumentSymbolsTest, SymbolsAreSorted) {
+ TestTU TU;
+ TU.ExtraArgs = {"-xobjective-c++", "-Wno-objc-root-class"};
+ Annotations Main(R"cpp(
+ @interface MYObject
+ @end
+
+ void someFunctionAbove() {}
+
+ @implementation MYObject
+ - (id)init { return self; }
+
+ void someHelperFunction() {}
+
+ - (void)retain {}
+ - (void)release {}
+ @end
+ )cpp");
+ TU.Code = Main.code().str();
+ EXPECT_THAT(getSymbols(TU.build()),
+ ElementsAre(AllOf(WithName("MYObject")),
+ AllOf(WithName("someFunctionAbove")),
+ // FIXME: This should be nested under MYObject below.
+ AllOf(WithName("someHelperFunction")),
+ AllOf(WithName("MYObject"),
+ Children(AllOf(WithName("-init")),
+ AllOf(WithName("-retain")),
+ AllOf(WithName("-release"))))));
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/TextMarks.h
===================================================================
--- /dev/null
+++ clang-tools-extra/clangd/TextMarks.h
@@ -0,0 +1,73 @@
+//===--- TextMarks.h ---------------------------------------------*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_TEXTMARKS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_TEXTMARKS_H
+
+#include "AST.h"
+#include "Protocol.h"
+#include "SourceCode.h"
+#include "clang/Basic/IdentifierTable.h"
+#include "clang/Lex/PPCallbacks.h"
+#include <string>
+
+namespace clang {
+namespace clangd {
+/// Represents a programmer specified mark/note, typically used to easily locate
+/// or differentiate code. e.g. a `pragma mark`, a `TODO`.
+///
+/// TextMarks occupy the entire line; it is not possible to have more than one
+/// `TextMark` per line.
+struct TextMark {
+ SourceLocation Loc;
+ std::string Text;
+ bool IsGroup;
+ DocumentSymbol toSymbol(const SourceManager &SM) const;
+};
+
+/// All `TextMarks` in the main file. Used to display the marks e.g. in the
+/// document symbols list.
+struct MainFileMarks {
+ /// Marks from the main file in order.
+ std::vector<TextMark> Marks;
+};
+
+/// Collect `#pragma mark` occurences in the main file.
+class CollectPragmaMarks : public PPCallbacks {
+public:
+ explicit CollectPragmaMarks(const SourceManager &SM, MainFileMarks &Out)
+ : SM(SM), Out(Out) {}
+ void PragmaMark(SourceLocation Loc, StringRef Trivia) override {
+ if (isInsideMainFile(Loc, SM)) {
+ StringRef Name = Trivia.trim();
+ bool IsGroup = false;
+ // "-\s+<group name>" or "<name>" after an initial trim. The former is
+ // considered a group, the latter just a mark. We need to include a name
+ // here since editors won't properly render the symbol otherwise.
+ StringRef MaybeGroupName = Name;
+ if (MaybeGroupName.consume_front("-") &&
+ (MaybeGroupName.ltrim() != MaybeGroupName ||
+ MaybeGroupName.empty())) {
+ Name =
+ MaybeGroupName.empty() ? "(unnamed group)" : MaybeGroupName.ltrim();
+ IsGroup = true;
+ } else if (Name.empty()) {
+ Name = "(unnamed mark)";
+ }
+ Out.Marks.emplace_back(TextMark{Loc, Name.str(), IsGroup});
+ }
+ }
+
+private:
+ const SourceManager &SM;
+ MainFileMarks &Out;
+};
+} // namespace clangd
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_TEXTMARKS_H
Index: clang-tools-extra/clangd/TextMarks.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clangd/TextMarks.cpp
@@ -0,0 +1,28 @@
+//===--- TextMarks.cpp -------------------------------------------*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "TextMarks.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Lex/Lexer.h"
+
+namespace clang {
+namespace clangd {
+
+DocumentSymbol TextMark::toSymbol(const SourceManager &SM) const {
+ DocumentSymbol Sym;
+ Sym.name = Text;
+ Sym.kind = SymbolKind::File;
+ Position Start = sourceLocToPosition(SM, Loc);
+ Position End = {Start.line + 1, 0};
+ Sym.range = Range{Start, End};
+ Sym.selectionRange = Range{Start, End};
+ return Sym;
+}
+
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/Protocol.h
===================================================================
--- clang-tools-extra/clangd/Protocol.h
+++ clang-tools-extra/clangd/Protocol.h
@@ -194,6 +194,14 @@
bool contains(Range Rng) const {
return start <= Rng.start && Rng.end <= end;
}
+
+ /// Expand this range to also contain `Rng`.
+ void mergeWith(Range Rng) {
+ if (Rng.start < start)
+ start = Rng.start;
+ if (end < Rng.end)
+ end = Rng.end;
+ }
};
bool fromJSON(const llvm::json::Value &, Range &, llvm::json::Path);
llvm::json::Value toJSON(const Range &);
Index: clang-tools-extra/clangd/ParsedAST.h
===================================================================
--- clang-tools-extra/clangd/ParsedAST.h
+++ clang-tools-extra/clangd/ParsedAST.h
@@ -25,6 +25,7 @@
#include "Diagnostics.h"
#include "Headers.h"
#include "Preamble.h"
+#include "TextMarks.h"
#include "index/CanonicalIncludes.h"
#include "support/Path.h"
#include "clang/Frontend/FrontendAction.h"
@@ -101,6 +102,8 @@
/// Gets all macro references (definition, expansions) present in the main
/// file, including those in the preamble region.
const MainFileMacros &getMacros() const;
+ /// Gets all textual marks in the main file.
+ const MainFileMarks &getMarks() const;
/// Tokens recorded while parsing the main file.
/// (!) does not have tokens from the preamble.
const syntax::TokenBuffer &getTokens() const { return Tokens; }
@@ -121,7 +124,8 @@
std::shared_ptr<const PreambleData> Preamble,
std::unique_ptr<CompilerInstance> Clang,
std::unique_ptr<FrontendAction> Action, syntax::TokenBuffer Tokens,
- MainFileMacros Macros, std::vector<Decl *> LocalTopLevelDecls,
+ MainFileMacros Macros, MainFileMarks Marks,
+ std::vector<Decl *> LocalTopLevelDecls,
llvm::Optional<std::vector<Diag>> Diags, IncludeStructure Includes,
CanonicalIncludes CanonIncludes);
@@ -144,6 +148,8 @@
/// All macro definitions and expansions in the main file.
MainFileMacros Macros;
+ // Textual marks in the main file (e.g. pragma marks, TODOs).
+ MainFileMarks Marks;
// Data, stored after parsing. None if AST was built with a stale preamble.
llvm::Optional<std::vector<Diag>> Diags;
// Top-level decls inside the current file. Not that this does not include
Index: clang-tools-extra/clangd/ParsedAST.cpp
===================================================================
--- clang-tools-extra/clangd/ParsedAST.cpp
+++ clang-tools-extra/clangd/ParsedAST.cpp
@@ -20,6 +20,7 @@
#include "IncludeFixer.h"
#include "Preamble.h"
#include "SourceCode.h"
+#include "TextMarks.h"
#include "TidyProvider.h"
#include "index/CanonicalIncludes.h"
#include "index/Index.h"
@@ -423,6 +424,10 @@
std::make_unique<CollectMainFileMacros>(Clang->getSourceManager(),
Macros));
+ MainFileMarks Marks;
+ Clang->getPreprocessor().addPPCallbacks(
+ std::make_unique<CollectPragmaMarks>(Clang->getSourceManager(), Marks));
+
// Copy over the includes from the preamble, then combine with the
// non-preamble includes below.
CanonicalIncludes CanonIncludes;
@@ -484,7 +489,7 @@
}
return ParsedAST(Inputs.Version, std::move(Preamble), std::move(Clang),
std::move(Action), std::move(Tokens), std::move(Macros),
- std::move(ParsedDecls), std::move(Diags),
+ std::move(Marks), std::move(ParsedDecls), std::move(Diags),
std::move(Includes), std::move(CanonIncludes));
}
@@ -524,6 +529,7 @@
}
const MainFileMacros &ParsedAST::getMacros() const { return Macros; }
+const MainFileMarks &ParsedAST::getMarks() const { return Marks; }
std::size_t ParsedAST::getUsedBytes() const {
auto &AST = getASTContext();
@@ -570,12 +576,14 @@
std::unique_ptr<CompilerInstance> Clang,
std::unique_ptr<FrontendAction> Action,
syntax::TokenBuffer Tokens, MainFileMacros Macros,
+ MainFileMarks Marks,
std::vector<Decl *> LocalTopLevelDecls,
llvm::Optional<std::vector<Diag>> Diags,
IncludeStructure Includes, CanonicalIncludes CanonIncludes)
: Version(Version), Preamble(std::move(Preamble)), Clang(std::move(Clang)),
Action(std::move(Action)), Tokens(std::move(Tokens)),
- Macros(std::move(Macros)), Diags(std::move(Diags)),
+ Macros(std::move(Macros)), Marks(std::move(Marks)),
+ Diags(std::move(Diags)),
LocalTopLevelDecls(std::move(LocalTopLevelDecls)),
Includes(std::move(Includes)), CanonIncludes(std::move(CanonIncludes)) {
Resolver = std::make_unique<HeuristicResolver>(getASTContext());
Index: clang-tools-extra/clangd/FindSymbols.cpp
===================================================================
--- clang-tools-extra/clangd/FindSymbols.cpp
+++ clang-tools-extra/clangd/FindSymbols.cpp
@@ -289,6 +289,10 @@
/// - visiting decls is actually simple, so we don't hit the complicated
/// cases that RAV mostly helps with (types, expressions, etc.)
class DocumentOutline {
+ struct TextMarkSymbol {
+ DocumentSymbol DocSym;
+ bool IsGroup;
+ };
// A DocumentSymbol we're constructing.
// We use this instead of DocumentSymbol directly so that we can keep track
// of the nodes we insert for macros.
@@ -303,16 +307,98 @@
DocumentSymbol build() && {
for (SymBuilder &C : Children) {
Symbol.children.push_back(std::move(C).build());
+ }
+ return std::move(Symbol);
+ }
+
+ /// Fix up ranges so each parent's range contains all of its descendants.
+ void standardizeRanges() {
+ for (SymBuilder &C : Children) {
+ C.standardizeRanges();
// Expand range to ensure children nest properly, which editors expect.
// This can fix some edge-cases in the AST, but is vital for macros.
// A macro expansion "contains" AST node if it covers the node's primary
// location, but it may not span the node's whole range.
- Symbol.range.start =
- std::min(Symbol.range.start, Symbol.children.back().range.start);
- Symbol.range.end =
- std::max(Symbol.range.end, Symbol.children.back().range.end);
+ Symbol.range.start = std::min(Symbol.range.start, C.Symbol.range.start);
+ Symbol.range.end = std::max(Symbol.range.end, C.Symbol.range.end);
+ }
+ // FIXME: We should also make sure nodes are properly sorted and nested
+ // here.
+ }
+
+ /// Merge in `TextMarks` which are sorted in descending order by line.
+ ///
+ /// This assumes this `SymBuilder` has standardized ranges and is
+ /// recursively sorted in ascending order by range.
+ void mergeTextMarks(std::vector<TextMarkSymbol> &TextMarks, bool IsRoot) {
+ size_t LastGroupIndex = 0;
+ size_t ChildIndex = 0;
+ bool HasOpenGroup = false;
+ while (!TextMarks.empty()) {
+ const auto &TextMark = TextMarks.back();
+ // If we don't contain the mark, we can break early, since it must
+ // be in a parent node instead. Note that we can still have children
+ // that need to be added to the last TextMark group though.
+ if (!Symbol.range.contains(TextMark.DocSym.range) && !IsRoot)
+ break;
+
+ while (ChildIndex < Children.size()) {
+ auto &C = Children[ChildIndex];
+
+ // Next mark is contained in the child, we need to recurse to let the
+ // child handle it.
+ if (C.Symbol.range.contains(TextMark.DocSym.range)) {
+ C.mergeTextMarks(TextMarks, /** IsRoot= */ false);
+ break;
+ }
+ // Text mark is after the child node, so we can process the child.
+ if (C.Symbol.range < TextMark.DocSym.range) {
+ if (HasOpenGroup) { // Child belongs to the previous TextMark group.
+ Children[LastGroupIndex].Children.push_back(C);
+ Children[LastGroupIndex].Symbol.range.mergeWith(C.Symbol.range);
+ Children.erase(Children.begin() + ChildIndex);
+ } else { // Child doesn't belong to a TextMark group. Skip it.
+ ChildIndex++;
+ }
+ continue;
+ }
+ // Text mark is before the child node.
+ SymBuilder Node;
+ Node.EnclosingMacroLoc = C.EnclosingMacroLoc;
+ Node.Symbol = TextMark.DocSym;
+ Children.insert(Children.begin() + ChildIndex, Node);
+ if (TextMark.IsGroup) {
+ LastGroupIndex = ChildIndex;
+ HasOpenGroup = true;
+ } else { // New text mark ends the previous group if one existed.
+ // FIXME: Consider letting non-groups nest inside a group.
+ HasOpenGroup = false;
+ }
+ TextMarks.pop_back();
+ ChildIndex++;
+ break;
+ }
+ // Reached the end of our children but still have TextMark(s) left,
+ // they should just be normal children.
+ if (ChildIndex == Children.size()) {
+ SymBuilder Node;
+ Node.EnclosingMacroLoc = EnclosingMacroLoc;
+ Node.Symbol = TextMark.DocSym;
+ Children.push_back(Node);
+ ChildIndex++;
+ TextMarks.pop_back();
+ }
+ }
+ // If we have a group open but still have children remaining, they belong
+ // to that group instead.
+ if (HasOpenGroup && ChildIndex < Children.size()) {
+ while (ChildIndex < Children.size()) {
+ auto &C = Children[ChildIndex];
+ Children[LastGroupIndex].Children.push_back(C);
+ Children[LastGroupIndex].Symbol.range.mergeWith(C.Symbol.range);
+ Children.erase(Children.begin() + ChildIndex);
+ }
}
- return std::move(Symbol);
}
// Add a symbol as a child of the current one.
@@ -379,6 +465,19 @@
SymBuilder Root;
for (auto &TopLevel : AST.getLocalTopLevelDecls())
traverseDecl(TopLevel, Root);
+
+ // We need to fix up the ranges before adding TextMarks to be sure we add
+ // them in the correct spots.
+ Root.standardizeRanges();
+
+ const auto &SM = AST.getSourceManager();
+ const auto &TextMarks = AST.getMarks().Marks;
+ std::vector<TextMarkSymbol> TextMarkSyms;
+ for (auto It = TextMarks.rbegin(); It != TextMarks.rend(); ++It) {
+ TextMarkSyms.push_back({It->toSymbol(SM), It->IsGroup});
+ }
+ Root.mergeTextMarks(TextMarkSyms, /** IsRoot= */ true);
+
return std::move(std::move(Root).build().children);
}
Index: clang-tools-extra/clangd/CMakeLists.txt
===================================================================
--- clang-tools-extra/clangd/CMakeLists.txt
+++ clang-tools-extra/clangd/CMakeLists.txt
@@ -94,6 +94,7 @@
SemanticSelection.cpp
SourceCode.cpp
QueryDriverDatabase.cpp
+ TextMarks.cpp
TidyProvider.cpp
TUScheduler.cpp
URI.cpp
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
