[clang-tools-extra] 0df1975 - [clangd] Value initialize SymbolIDs
Kadir Cetinkaya via cfe-commits
cfe-commits at lists.llvm.org
Mon Nov 2 02:43:39 PST 2020
Author: Kadir Cetinkaya
Date: 2020-11-02T11:37:47+01:00
New Revision: 0df197516b69a4477a4c8f02b7c4dccacda5f23f
URL: https://github.com/llvm/llvm-project/commit/0df197516b69a4477a4c8f02b7c4dccacda5f23f
DIFF: https://github.com/llvm/llvm-project/commit/0df197516b69a4477a4c8f02b7c4dccacda5f23f.diff
LOG: [clangd] Value initialize SymbolIDs
We were default initializing SymbolIDs before, which would leave
indeterminate values in underlying std::array.
This patch updates the underlying data initalization to be value-init and adds a
way to check for validness of a SymbolID.
Differential Revision: https://reviews.llvm.org/D90397
Added:
Modified:
clang-tools-extra/clangd/AST.cpp
clang-tools-extra/clangd/AST.h
clang-tools-extra/clangd/CodeComplete.cpp
clang-tools-extra/clangd/CollectMacros.cpp
clang-tools-extra/clangd/HeaderSourceSwitch.cpp
clang-tools-extra/clangd/Hover.cpp
clang-tools-extra/clangd/IncludeFixer.cpp
clang-tools-extra/clangd/Protocol.cpp
clang-tools-extra/clangd/Protocol.h
clang-tools-extra/clangd/XRefs.cpp
clang-tools-extra/clangd/index/SymbolCollector.cpp
clang-tools-extra/clangd/index/SymbolID.h
clang-tools-extra/clangd/refactor/Rename.cpp
clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp
clang-tools-extra/clangd/unittests/HoverTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/AST.cpp b/clang-tools-extra/clangd/AST.cpp
index ce1254f6693c..d1702b2d25b6 100644
--- a/clang-tools-extra/clangd/AST.cpp
+++ b/clang-tools-extra/clangd/AST.cpp
@@ -282,21 +282,20 @@ std::string printNamespaceScope(const DeclContext &DC) {
return "";
}
-llvm::Optional<SymbolID> getSymbolID(const Decl *D) {
+SymbolID getSymbolID(const Decl *D) {
llvm::SmallString<128> USR;
if (index::generateUSRForDecl(D, USR))
- return None;
+ return {};
return SymbolID(USR);
}
-llvm::Optional<SymbolID> getSymbolID(const llvm::StringRef MacroName,
- const MacroInfo *MI,
- const SourceManager &SM) {
+SymbolID getSymbolID(const llvm::StringRef MacroName, const MacroInfo *MI,
+ const SourceManager &SM) {
if (MI == nullptr)
- return None;
+ return {};
llvm::SmallString<128> USR;
if (index::generateUSRForMacro(MacroName, MI->getDefinitionLoc(), SM, USR))
- return None;
+ return {};
return SymbolID(USR);
}
diff --git a/clang-tools-extra/clangd/AST.h b/clang-tools-extra/clangd/AST.h
index c8b6008339e2..1e3447376c10 100644
--- a/clang-tools-extra/clangd/AST.h
+++ b/clang-tools-extra/clangd/AST.h
@@ -64,19 +64,18 @@ std::string printName(const ASTContext &Ctx, const NamedDecl &ND);
/// string if decl is not a template specialization.
std::string printTemplateSpecializationArgs(const NamedDecl &ND);
-/// Gets the symbol ID for a declaration, if possible.
-llvm::Optional<SymbolID> getSymbolID(const Decl *D);
+/// Gets the symbol ID for a declaration. Returned SymbolID might be null.
+SymbolID getSymbolID(const Decl *D);
-/// Gets the symbol ID for a macro, if possible.
+/// Gets the symbol ID for a macro. Returned SymbolID might be null.
/// Currently, this is an encoded USR of the macro, which incorporates macro
/// locations (e.g. file name, offset in file).
/// FIXME: the USR semantics might not be stable enough as the ID for index
/// macro (e.g. a change in definition offset can result in a
diff erent USR). We
/// could change these semantics in the future by reimplementing this funcure
/// (e.g. avoid USR for macros).
-llvm::Optional<SymbolID> getSymbolID(const llvm::StringRef MacroName,
- const MacroInfo *MI,
- const SourceManager &SM);
+SymbolID getSymbolID(const llvm::StringRef MacroName, const MacroInfo *MI,
+ const SourceManager &SM);
/// Returns a QualType as string. The result doesn't contain unwritten scopes
/// like anonymous/inline namespace.
diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang-tools-extra/clangd/CodeComplete.cpp
index 8396de635135..24678a3cc1bc 100644
--- a/clang-tools-extra/clangd/CodeComplete.cpp
+++ b/clang-tools-extra/clangd/CodeComplete.cpp
@@ -503,20 +503,19 @@ struct CodeCompletionBuilder {
};
// Determine the symbol ID for a Sema code completion result, if possible.
-llvm::Optional<SymbolID> getSymbolID(const CodeCompletionResult &R,
- const SourceManager &SM) {
+SymbolID getSymbolID(const CodeCompletionResult &R, const SourceManager &SM) {
switch (R.Kind) {
case CodeCompletionResult::RK_Declaration:
case CodeCompletionResult::RK_Pattern: {
// Computing USR caches linkage, which may change after code completion.
if (hasUnstableLinkage(R.Declaration))
- return llvm::None;
+ return {};
return clang::clangd::getSymbolID(R.Declaration);
}
case CodeCompletionResult::RK_Macro:
return clang::clangd::getSymbolID(R.Macro->getName(), R.MacroDefInfo, SM);
case CodeCompletionResult::RK_Keyword:
- return None;
+ return {};
}
llvm_unreachable("unknown CodeCompletionResult kind");
}
@@ -1586,7 +1585,7 @@ class CodeCompleteFlow {
[&](const CodeCompletionResult &SemaResult) -> const Symbol * {
if (auto SymID =
getSymbolID(SemaResult, Recorder->CCSema->getSourceManager())) {
- auto I = IndexResults.find(*SymID);
+ auto I = IndexResults.find(SymID);
if (I != IndexResults.end()) {
UsedIndexResults.insert(&*I);
return &*I;
diff --git a/clang-tools-extra/clangd/CollectMacros.cpp b/clang-tools-extra/clangd/CollectMacros.cpp
index ea7dd18ee130..bd6165ef10ba 100644
--- a/clang-tools-extra/clangd/CollectMacros.cpp
+++ b/clang-tools-extra/clangd/CollectMacros.cpp
@@ -26,7 +26,7 @@ void CollectMainFileMacros::add(const Token &MacroNameTok,
auto Range = halfOpenToRange(
SM, CharSourceRange::getCharRange(Loc, MacroNameTok.getEndLoc()));
if (auto SID = getSymbolID(Name, MI, SM))
- Out.MacroRefs[*SID].push_back(Range);
+ Out.MacroRefs[SID].push_back(Range);
else
Out.UnknownMacros.push_back(Range);
}
diff --git a/clang-tools-extra/clangd/HeaderSourceSwitch.cpp b/clang-tools-extra/clangd/HeaderSourceSwitch.cpp
index 3ebf0542dcf0..d6b0b98f8d42 100644
--- a/clang-tools-extra/clangd/HeaderSourceSwitch.cpp
+++ b/clang-tools-extra/clangd/HeaderSourceSwitch.cpp
@@ -80,7 +80,7 @@ llvm::Optional<Path> getCorrespondingHeaderOrSource(const Path &OriginalFile,
// Find all symbols present in the original file.
for (const auto *D : getIndexableLocalDecls(AST)) {
if (auto ID = getSymbolID(D))
- Request.IDs.insert(*ID);
+ Request.IDs.insert(ID);
}
llvm::StringMap<int> Candidates; // Target path => score.
auto AwardTarget = [&](const char *TargetURI) {
diff --git a/clang-tools-extra/clangd/Hover.cpp b/clang-tools-extra/clangd/Hover.cpp
index 9da8a9c9af57..d599ccb557c2 100644
--- a/clang-tools-extra/clangd/Hover.cpp
+++ b/clang-tools-extra/clangd/Hover.cpp
@@ -269,7 +269,7 @@ void enhanceFromIndex(HoverInfo &Hover, const NamedDecl &ND,
if (!ID)
return;
LookupRequest Req;
- Req.IDs.insert(*ID);
+ Req.IDs.insert(ID);
Index->lookup(Req, [&](const Symbol &S) {
Hover.Documentation = std::string(S.Documentation);
});
diff --git a/clang-tools-extra/clangd/IncludeFixer.cpp b/clang-tools-extra/clangd/IncludeFixer.cpp
index e2bbb6c9c0dc..ab4a235a662b 100644
--- a/clang-tools-extra/clangd/IncludeFixer.cpp
+++ b/clang-tools-extra/clangd/IncludeFixer.cpp
@@ -133,7 +133,7 @@ std::vector<Fix> IncludeFixer::fixIncompleteType(const Type &T) const {
auto ID = getSymbolID(TD);
if (!ID)
return {};
- llvm::Optional<const SymbolSlab *> Symbols = lookupCached(*ID);
+ llvm::Optional<const SymbolSlab *> Symbols = lookupCached(ID);
if (!Symbols)
return {};
const SymbolSlab &Syms = **Symbols;
diff --git a/clang-tools-extra/clangd/Protocol.cpp b/clang-tools-extra/clangd/Protocol.cpp
index d11307d04cee..2a457420ff21 100644
--- a/clang-tools-extra/clangd/Protocol.cpp
+++ b/clang-tools-extra/clangd/Protocol.cpp
@@ -701,8 +701,8 @@ llvm::json::Value toJSON(const SymbolDetails &P) {
if (!P.USR.empty())
Result["usr"] = P.USR;
- if (P.ID.hasValue())
- Result["id"] = P.ID.getValue().str();
+ if (P.ID)
+ Result["id"] = P.ID.str();
// FIXME: workaround for older gcc/clang
return std::move(Result);
diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h
index f846accbdde7..d0c4c6bbd4b4 100644
--- a/clang-tools-extra/clangd/Protocol.h
+++ b/clang-tools-extra/clangd/Protocol.h
@@ -1053,7 +1053,7 @@ struct SymbolDetails {
/// (See USRGeneration.h)
std::string USR;
- llvm::Optional<SymbolID> ID;
+ SymbolID ID;
};
llvm::json::Value toJSON(const SymbolDetails &);
llvm::raw_ostream &operator<<(llvm::raw_ostream &, const SymbolDetails &);
diff --git a/clang-tools-extra/clangd/XRefs.cpp b/clang-tools-extra/clangd/XRefs.cpp
index 02dbd3ee9278..db61452e0f17 100644
--- a/clang-tools-extra/clangd/XRefs.cpp
+++ b/clang-tools-extra/clangd/XRefs.cpp
@@ -321,7 +321,7 @@ locateASTReferent(SourceLocation CurLoc, const syntax::Token *TouchedIdentifier,
// Record SymbolID for index lookup later.
if (auto ID = getSymbolID(D))
- ResultIndex[*ID] = Result.size() - 1;
+ ResultIndex[ID] = Result.size() - 1;
};
// Emit all symbol locations (declaration or definition) from AST.
@@ -1154,7 +1154,7 @@ ReferencesResult findReferences(ParsedAST &AST, Position Pos, uint32_t Limit,
if (auto MacroSID = getSymbolID(Macro->Name, Macro->Info, SM)) {
// Collect macro references from main file.
const auto &IDToRefs = AST.getMacros().MacroRefs;
- auto Refs = IDToRefs.find(*MacroSID);
+ auto Refs = IDToRefs.find(MacroSID);
if (Refs != IDToRefs.end()) {
for (const auto &Ref : Refs->second) {
Location Result;
@@ -1163,7 +1163,7 @@ ReferencesResult findReferences(ParsedAST &AST, Position Pos, uint32_t Limit,
Results.References.push_back(std::move(Result));
}
}
- Req.IDs.insert(*MacroSID);
+ Req.IDs.insert(MacroSID);
}
} else {
// Handle references to Decls.
@@ -1200,7 +1200,7 @@ ReferencesResult findReferences(ParsedAST &AST, Position Pos, uint32_t Limit,
if (D->getParentFunctionOrMethod())
continue;
if (auto ID = getSymbolID(D))
- Req.IDs.insert(*ID);
+ Req.IDs.insert(ID);
}
}
}
@@ -1332,9 +1332,8 @@ declToTypeHierarchyItem(ASTContext &Ctx, const NamedDecl &ND) {
// This allows typeHierarchy/resolve to be used to
// resolve children of items returned in a previous request
// for parents.
- if (auto ID = getSymbolID(&ND)) {
- THI.data = ID->str();
- }
+ if (auto ID = getSymbolID(&ND))
+ THI.data = ID.str();
return THI;
}
@@ -1539,8 +1538,8 @@ getTypeHierarchy(ParsedAST &AST, Position Pos, int ResolveLevels,
Result->children.emplace();
if (Index) {
- if (Optional<SymbolID> ID = getSymbolID(CXXRD))
- fillSubTypes(*ID, *Result->children, Index, ResolveLevels, TUPath);
+ if (auto ID = getSymbolID(CXXRD))
+ fillSubTypes(ID, *Result->children, Index, ResolveLevels, TUPath);
}
}
diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp
index e7d44a79eb47..511b5a2c6047 100644
--- a/clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ b/clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -324,7 +324,7 @@ bool SymbolCollector::handleDeclOccurrence(
// refs, because the indexing code only populates relations for specific
// occurrences. For example, RelationBaseOf is only populated for the
// occurrence inside the base-specifier.
- processRelations(*ND, *ID, Relations);
+ processRelations(*ND, ID, Relations);
bool CollectRef = static_cast<bool>(Opts.RefFilter & toRefKind(Roles));
bool IsOnlyRef =
@@ -356,15 +356,15 @@ bool SymbolCollector::handleDeclOccurrence(
if (!OriginalDecl)
return true;
- const Symbol *BasicSymbol = Symbols.find(*ID);
+ const Symbol *BasicSymbol = Symbols.find(ID);
if (isPreferredDeclaration(*OriginalDecl, Roles))
// If OriginalDecl is preferred, replace/create the existing canonical
// declaration (e.g. a class forward declaration). There should be at most
// one duplicate as we expect to see only one preferred declaration per
// TU, because in practice they are definitions.
- BasicSymbol = addDeclaration(*OriginalDecl, std::move(*ID), IsMainFileOnly);
+ BasicSymbol = addDeclaration(*OriginalDecl, std::move(ID), IsMainFileOnly);
else if (!BasicSymbol || DeclIsCanonical)
- BasicSymbol = addDeclaration(*ND, std::move(*ID), IsMainFileOnly);
+ BasicSymbol = addDeclaration(*ND, std::move(ID), IsMainFileOnly);
if (Roles & static_cast<unsigned>(index::SymbolRole::Definition))
addDefinition(*OriginalDecl, *BasicSymbol);
@@ -422,7 +422,7 @@ bool SymbolCollector::handleMacroOccurrence(const IdentifierInfo *Name,
// Do not store references to main-file macros.
if ((static_cast<unsigned>(Opts.RefFilter) & Roles) && !IsMainFileOnly &&
(Opts.RefsInHeaders || SM.getFileID(SpellingLoc) == SM.getMainFileID()))
- MacroRefs[*ID].push_back({Loc, Roles});
+ MacroRefs[ID].push_back({Loc, Roles});
// Collect symbols.
if (!Opts.CollectMacro)
@@ -447,11 +447,11 @@ bool SymbolCollector::handleMacroOccurrence(const IdentifierInfo *Name,
return true;
// Only collect one instance in case there are multiple.
- if (Symbols.find(*ID) != nullptr)
+ if (Symbols.find(ID) != nullptr)
return true;
Symbol S;
- S.ID = std::move(*ID);
+ S.ID = std::move(ID);
S.Name = Name->getName();
if (!IsMainFileOnly) {
S.Flags |= Symbol::IndexedForCodeCompletion;
@@ -507,7 +507,7 @@ void SymbolCollector::processRelations(
// in the index and find nothing, but that's a situation they
// probably need to handle for other reasons anyways.
// We currently do (B) because it's simpler.
- this->Relations.insert(Relation{ID, RelationKind::BaseOf, *ObjectID});
+ this->Relations.insert(Relation{ID, RelationKind::BaseOf, ObjectID});
}
}
@@ -531,7 +531,7 @@ void SymbolCollector::finish() {
};
for (const NamedDecl *ND : ReferencedDecls) {
if (auto ID = getSymbolID(ND)) {
- IncRef(*ID);
+ IncRef(ID);
}
}
if (Opts.CollectMacro) {
@@ -541,13 +541,13 @@ void SymbolCollector::finish() {
if (const auto *MI = PP->getMacroDefinition(II).getMacroInfo())
if (auto ID = getSymbolID(II->getName(), MI, PP->getSourceManager()))
if (MI->isUsedForHeaderGuard())
- Symbols.erase(*ID);
+ Symbols.erase(ID);
}
// Now increment refcounts.
for (const IdentifierInfo *II : ReferencedMacros) {
if (const auto *MI = PP->getMacroDefinition(II).getMacroInfo())
if (auto ID = getSymbolID(II->getName(), MI, PP->getSourceManager()))
- IncRef(*ID);
+ IncRef(ID);
}
}
// Fill in IncludeHeaders.
@@ -624,7 +624,7 @@ void SymbolCollector::finish() {
NameKind == DeclarationName::CXXConstructorName;
bool Spelled = IdentifierToken && IsTargetKind &&
Name.getAsString() == IdentifierToken->text(SM);
- CollectRef(*ID, LocAndRole, Spelled);
+ CollectRef(ID, LocAndRole, Spelled);
}
}
}
diff --git a/clang-tools-extra/clangd/index/SymbolID.h b/clang-tools-extra/clangd/index/SymbolID.h
index ec7e73301a68..518c130830d5 100644
--- a/clang-tools-extra/clangd/index/SymbolID.h
+++ b/clang-tools-extra/clangd/index/SymbolID.h
@@ -15,6 +15,7 @@
#include "llvm/Support/Error.h"
#include "llvm/Support/raw_ostream.h"
#include <array>
+#include <cstdint>
#include <string>
namespace clang {
@@ -53,8 +54,11 @@ class SymbolID {
std::string str() const;
static llvm::Expected<SymbolID> fromStr(llvm::StringRef);
+ bool isNull() const { return *this == SymbolID(); }
+ explicit operator bool() const { return !isNull(); }
+
private:
- std::array<uint8_t, RawSize> HashValue;
+ std::array<uint8_t, RawSize> HashValue{};
};
llvm::hash_code hash_value(const SymbolID &ID);
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index e072853dac9f..7ec29d5b086b 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -63,7 +63,7 @@ llvm::Optional<std::string> getOtherRefFile(const Decl &D, StringRef MainFile,
// tradeoff. We expect the number of symbol references in the current file
// is smaller than the limit.
Req.Limit = 100;
- Req.IDs.insert(*getSymbolID(&D));
+ Req.IDs.insert(getSymbolID(&D));
llvm::Optional<std::string> OtherFile;
Index.refs(Req, [&](const Ref &R) {
if (OtherFile)
@@ -244,7 +244,7 @@ std::vector<SourceLocation> findOccurrencesWithinFile(ParsedAST &AST,
return;
for (const auto *Target : Ref.Targets) {
auto ID = getSymbolID(Target);
- if (!ID || TargetIDs.find(*ID) == TargetIDs.end())
+ if (!ID || TargetIDs.find(ID) == TargetIDs.end())
return;
}
Results.push_back(Ref.NameLoc);
@@ -304,7 +304,7 @@ findOccurrencesOutsideFile(const NamedDecl &RenameDecl,
size_t MaxLimitFiles) {
trace::Span Tracer("FindOccurrencesOutsideFile");
RefsRequest RQuest;
- RQuest.IDs.insert(*getSymbolID(&RenameDecl));
+ RQuest.IDs.insert(getSymbolID(&RenameDecl));
// Absolute file path => rename occurrences in that file.
llvm::StringMap<std::vector<Range>> AffectedFiles;
diff --git a/clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp b/clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp
index aa4c0190b03d..4576fa9b8eb0 100644
--- a/clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp
+++ b/clang-tools-extra/clangd/unittests/CollectMacrosTests.cpp
@@ -96,7 +96,7 @@ TEST(CollectMainFileMacros, SelectedMacros) {
auto SID = getSymbolID(Macro->Name, Macro->Info, SM);
EXPECT_THAT(ExpectedRefs,
- UnorderedElementsAreArray(ActualMacroRefs.MacroRefs[*SID]))
+ UnorderedElementsAreArray(ActualMacroRefs.MacroRefs[SID]))
<< "Annotation=" << I << ", MacroName=" << Macro->Name
<< ", Test = " << Test;
}
diff --git a/clang-tools-extra/clangd/unittests/HoverTests.cpp b/clang-tools-extra/clangd/unittests/HoverTests.cpp
index 87a1b42315b3..fdebf0da6710 100644
--- a/clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ b/clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -2077,7 +2077,7 @@ TEST(Hover, DocsFromIndex) {
TestTU TU = TestTU::withCode(T.code());
auto AST = TU.build();
Symbol IndexSym;
- IndexSym.ID = *getSymbolID(&findDecl(AST, "X"));
+ IndexSym.ID = getSymbolID(&findDecl(AST, "X"));
IndexSym.Documentation = "comment from index";
SymbolSlab::Builder Symbols;
Symbols.insert(IndexSym);
More information about the cfe-commits
mailing list