[PATCH] D69840: [Basic] Make SourceLocation usable as key in hash maps, NFCI
miyuki added a comment. Yes, I am still interested in working on this. For now, I've split out some changes that don't require any modifications in `SourceLocation` into https://reviews.llvm.org/D89705. > A general comment is that there are a couple of functional changes in this > patch, where hash values are changing and data structures are being changed, > but they're buried in the noise of the refactoring that's going on at the > same time. I suggest splitting this up at least into two Will do. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69840/new/ https://reviews.llvm.org/D69840 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69840: [Basic] Make SourceLocation usable as key in hash maps, NFCI
dexonsmith requested changes to this revision.
dexonsmith added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: sstefan1.
If you're still interested in pursuing this, I'm happy to review it.
A general comment is that there are a couple of functional changes in this
patch, where hash values are changing and data structures are being changed,
but they're buried in the noise of the refactoring that's going on at the same
time. I suggest splitting this up at least into two, where most of the NFC
changes are in a different commit.
Comment at:
clang-tools-extra/clang-tidy/abseil/UpgradeDurationConversionsCheck.h:35
private:
- std::unordered_set MatchedTemplateLocations;
+ llvm::DenseSet MatchedTemplateLocations;
};
Changing the data structure (`DenseSet` <= `std::unordered_set`) seems
unrelated to the rest of the patch. If it's necessary / useful, then please do
it separately. It'll be important to confirm that the users of
`MatchedTemplateLocations` don't rely on the values having stable addresses.
Comment at: clang/include/clang/Basic/SourceLocation.h:178
+ unsigned getHashValue() const {
+return ID * 37U;
I suggest reusing the hash function for `unsigned`.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69840/new/
https://reviews.llvm.org/D69840
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69840: [Basic] Make SourceLocation usable as key in hash maps, NFCI
miyuki updated this revision to Diff 249443.
miyuki added a comment.
Herald added subscribers: usaxena95, kadircet, arphaman, jkorous, kbarton,
nemanjai.
Rebased, updated several more hash map occurrences.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69840/new/
https://reviews.llvm.org/D69840
Files:
clang-tools-extra/clang-tidy/abseil/UpgradeDurationConversionsCheck.cpp
clang-tools-extra/clang-tidy/abseil/UpgradeDurationConversionsCheck.h
clang-tools-extra/clang-tidy/cppcoreguidelines/SpecialMemberFunctionsCheck.h
clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp
clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.h
clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h
clang-tools-extra/clangd/FindTarget.cpp
clang/include/clang/Basic/SourceLocation.h
clang/include/clang/Edit/EditedSource.h
clang/include/clang/Sema/Sema.h
clang/include/clang/Tooling/Syntax/Tokens.h
clang/lib/ARCMigrate/TransGCAttrs.cpp
clang/lib/ARCMigrate/TransProperties.cpp
clang/lib/ARCMigrate/Transforms.h
clang/lib/CodeGen/CGOpenMPRuntime.cpp
clang/lib/CodeGen/CGOpenMPRuntime.h
clang/lib/Edit/EditedSource.cpp
clang/lib/Frontend/Rewrite/InclusionRewriter.cpp
clang/lib/Tooling/Syntax/BuildTree.cpp
clang/lib/Tooling/Syntax/Tokens.cpp
Index: clang/lib/Tooling/Syntax/Tokens.cpp
===
--- clang/lib/Tooling/Syntax/Tokens.cpp
+++ clang/lib/Tooling/Syntax/Tokens.cpp
@@ -379,7 +379,7 @@
Collector->PP.getSourceManager().isBeforeInTranslationUnit(
Range.getBegin(), LastExpansionEnd)))
return;
-Collector->Expansions[Range.getBegin().getRawEncoding()] = Range.getEnd();
+Collector->Expansions[Range.getBegin()] = Range.getEnd();
LastExpansionEnd = Range.getEnd();
}
// FIXME: handle directives like #pragma, #include, etc.
@@ -606,7 +606,7 @@
auto L = File.SpelledTokens[NextSpelled].location();
if (Offset <= SM.getFileOffset(L))
return llvm::None; // reached the offset we are looking for.
- auto Mapping = CollectedExpansions.find(L.getRawEncoding());
+ auto Mapping = CollectedExpansions.find(L);
if (Mapping != CollectedExpansions.end())
return Mapping->second; // found a mapping before the offset.
}
Index: clang/lib/Tooling/Syntax/BuildTree.cpp
===
--- clang/lib/Tooling/Syntax/BuildTree.cpp
+++ clang/lib/Tooling/Syntax/BuildTree.cpp
@@ -53,7 +53,7 @@
public:
TreeBuilder(syntax::Arena ) : Arena(Arena), Pending(Arena) {
for (const auto : Arena.tokenBuffer().expandedTokens())
- LocationToToken.insert({T.location().getRawEncoding(), });
+ LocationToToken.insert({T.location(), });
}
llvm::BumpPtrAllocator () { return Arena.allocator(); }
@@ -310,8 +310,7 @@
syntax::Arena
/// To quickly find tokens by their start location.
- llvm::DenseMap
- LocationToToken;
+ llvm::DenseMap LocationToToken;
Forest Pending;
llvm::DenseSet DeclsWithoutSemicolons;
};
@@ -653,7 +652,7 @@
}
const syntax::Token *syntax::TreeBuilder::findToken(SourceLocation L) const {
- auto It = LocationToToken.find(L.getRawEncoding());
+ auto It = LocationToToken.find(L);
assert(It != LocationToToken.end());
return It->second;
}
Index: clang/lib/Frontend/Rewrite/InclusionRewriter.cpp
===
--- clang/lib/Frontend/Rewrite/InclusionRewriter.cpp
+++ clang/lib/Frontend/Rewrite/InclusionRewriter.cpp
@@ -44,13 +44,13 @@
bool ShowLineMarkers; ///< Show #line markers.
bool UseLineDirectives; ///< Use of line directives or line markers.
/// Tracks where inclusions that change the file are found.
- std::map FileIncludes;
+ std::map FileIncludes;
/// Tracks where inclusions that import modules are found.
- std::map ModuleIncludes;
+ std::map ModuleIncludes;
/// Tracks where inclusions that enter modules (in a module build) are found.
- std::map ModuleEntryIncludes;
+ std::map ModuleEntryIncludes;
/// Tracks where #if and #elif directives get evaluated and whether to true.
- std::map IfConditions;
+ std::map IfConditions;
/// Used transitively for building up the FileIncludes mapping over the
/// various \c PPCallbacks callbacks.
SourceLocation LastInclusionLocation;
@@ -65,7 +65,7 @@
void detectMainFileEOL();
void handleModuleBegin(Token ) {
assert(Tok.getKind() == tok::annot_module_begin);
-ModuleEntryIncludes.insert({Tok.getLocation().getRawEncoding(),
+ModuleEntryIncludes.insert({Tok.getLocation(),
(Module *)Tok.getAnnotationValue()});
}
private:
@@ -164,7 +164,7 @@
return;
FileID Id = FullSourceLoc(Loc, SM).getFileID();
auto P = FileIncludes.insert(
-
[PATCH] D69840: [Basic] Make SourceLocation usable as key in hash maps, NFCI
miyuki added a comment. ping^4 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69840/new/ https://reviews.llvm.org/D69840 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69840: [Basic] Make SourceLocation usable as key in hash maps, NFCI
aprantl resigned from this revision. aprantl added a comment. @rsmith can you take a look at this to see if this is going into the right direction? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69840/new/ https://reviews.llvm.org/D69840 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69840: [Basic] Make SourceLocation usable as key in hash maps, NFCI
miyuki added a comment. ping^3 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69840/new/ https://reviews.llvm.org/D69840 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69840: [Basic] Make SourceLocation usable as key in hash maps, NFCI
miyuki added a comment. ping^2 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69840/new/ https://reviews.llvm.org/D69840 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69840: [Basic] Make SourceLocation usable as key in hash maps, NFCI
miyuki added a comment. ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69840/new/ https://reviews.llvm.org/D69840 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69840: [Basic] Make SourceLocation usable as key in hash maps, NFCI
miyuki added a comment. The motivation is to be able to make source locations' underlying type configurable. Richard Smith suggested that this might be feasible: http://lists.llvm.org/pipermail/cfe-dev/2019-October/063515.html So, the first step is to get rid of getRawEncoding where possible. I think this would make the code cleaner anyway. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69840/new/ https://reviews.llvm.org/D69840 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69840: [Basic] Make SourceLocation usable as key in hash maps, NFCI
aprantl added a comment. What's the motivation behind this? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69840/new/ https://reviews.llvm.org/D69840 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D69840: [Basic] Make SourceLocation usable as key in hash maps, NFCI
miyuki updated this revision to Diff 227894.
miyuki added a comment.
Changed getRawEncoding -> getHashValue in Sema.h
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69840/new/
https://reviews.llvm.org/D69840
Files:
clang/include/clang/Basic/SourceLocation.h
clang/include/clang/Edit/EditedSource.h
clang/include/clang/Sema/Sema.h
clang/include/clang/Tooling/Syntax/Tokens.h
clang/lib/ARCMigrate/TransGCAttrs.cpp
clang/lib/ARCMigrate/TransProperties.cpp
clang/lib/ARCMigrate/Transforms.h
clang/lib/CodeGen/CGOpenMPRuntime.cpp
clang/lib/CodeGen/CGOpenMPRuntime.h
clang/lib/Edit/EditedSource.cpp
clang/lib/Frontend/Rewrite/InclusionRewriter.cpp
clang/lib/Tooling/Syntax/Tokens.cpp
Index: clang/lib/Tooling/Syntax/Tokens.cpp
===
--- clang/lib/Tooling/Syntax/Tokens.cpp
+++ clang/lib/Tooling/Syntax/Tokens.cpp
@@ -297,7 +297,7 @@
Collector->PP.getSourceManager().isBeforeInTranslationUnit(
Range.getBegin(), LastExpansionEnd)))
return;
-Collector->Expansions[Range.getBegin().getRawEncoding()] = Range.getEnd();
+Collector->Expansions[Range.getBegin()] = Range.getEnd();
LastExpansionEnd = Range.getEnd();
}
// FIXME: handle directives like #pragma, #include, etc.
@@ -524,7 +524,7 @@
auto L = File.SpelledTokens[NextSpelled].location();
if (Offset <= SM.getFileOffset(L))
return llvm::None; // reached the offset we are looking for.
- auto Mapping = CollectedExpansions.find(L.getRawEncoding());
+ auto Mapping = CollectedExpansions.find(L);
if (Mapping != CollectedExpansions.end())
return Mapping->second; // found a mapping before the offset.
}
Index: clang/lib/Frontend/Rewrite/InclusionRewriter.cpp
===
--- clang/lib/Frontend/Rewrite/InclusionRewriter.cpp
+++ clang/lib/Frontend/Rewrite/InclusionRewriter.cpp
@@ -44,13 +44,13 @@
bool ShowLineMarkers; ///< Show #line markers.
bool UseLineDirectives; ///< Use of line directives or line markers.
/// Tracks where inclusions that change the file are found.
- std::map FileIncludes;
+ std::map FileIncludes;
/// Tracks where inclusions that import modules are found.
- std::map ModuleIncludes;
+ std::map ModuleIncludes;
/// Tracks where inclusions that enter modules (in a module build) are found.
- std::map ModuleEntryIncludes;
+ std::map ModuleEntryIncludes;
/// Tracks where #if and #elif directives get evaluated and whether to true.
- std::map IfConditions;
+ std::map IfConditions;
/// Used transitively for building up the FileIncludes mapping over the
/// various \c PPCallbacks callbacks.
SourceLocation LastInclusionLocation;
@@ -65,7 +65,7 @@
void detectMainFileEOL();
void handleModuleBegin(Token ) {
assert(Tok.getKind() == tok::annot_module_begin);
-ModuleEntryIncludes.insert({Tok.getLocation().getRawEncoding(),
+ModuleEntryIncludes.insert({Tok.getLocation(),
(Module *)Tok.getAnnotationValue()});
}
private:
@@ -164,7 +164,7 @@
return;
FileID Id = FullSourceLoc(Loc, SM).getFileID();
auto P = FileIncludes.insert(
- std::make_pair(LastInclusionLocation.getRawEncoding(),
+ std::make_pair(LastInclusionLocation,
IncludedFile(Id, NewFileType, PP.GetCurDirLookup(;
(void)P;
assert(P.second && "Unexpected revisitation of the same include directive");
@@ -199,8 +199,7 @@
const Module *Imported,
SrcMgr::CharacteristicKind FileType){
if (Imported) {
-auto P = ModuleIncludes.insert(
-std::make_pair(HashLoc.getRawEncoding(), Imported));
+auto P = ModuleIncludes.insert(std::make_pair(HashLoc, Imported));
(void)P;
assert(P.second && "Unexpected revisitation of the same include directive");
} else
@@ -209,8 +208,7 @@
void InclusionRewriter::If(SourceLocation Loc, SourceRange ConditionRange,
ConditionValueKind ConditionValue) {
- auto P = IfConditions.insert(
- std::make_pair(Loc.getRawEncoding(), ConditionValue == CVK_True));
+ auto P = IfConditions.insert(std::make_pair(Loc, ConditionValue == CVK_True));
(void)P;
assert(P.second && "Unexpected revisitation of the same if directive");
}
@@ -218,8 +216,7 @@
void InclusionRewriter::Elif(SourceLocation Loc, SourceRange ConditionRange,
ConditionValueKind ConditionValue,
SourceLocation IfLoc) {
- auto P = IfConditions.insert(
- std::make_pair(Loc.getRawEncoding(), ConditionValue == CVK_True));
+ auto P = IfConditions.insert(std::make_pair(Loc, ConditionValue == CVK_True));
(void)P;
assert(P.second && "Unexpected revisitation of the same elif directive");
}
@@ -228,7 +225,7 @@
/// an inclusion directive) in the map of
[PATCH] D69840: [Basic] Make SourceLocation usable as key in hash maps, NFCI
miyuki created this revision.
miyuki added reviewers: aprantl, rsmith, faisalv.
Herald added a reviewer: jdoerfert.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
miyuki edited reviewers, added: JDevlieghere; removed: jdoerfert.
Herald added a reviewer: jdoerfert.
This change creates a DenseMapInfo trait specialization for the
SourceLocation class. The empty key, the tombstone key and the hash
function are identical to DenseMapInfo, because we already
have hash maps that use raw the representation of SourceLocation as
a key.
The patch also converts the existing llvm::DenseMap,
llvm::DenseSet and std::map objects that store
source location as 'unsigned' to using SourceLocation directly.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D69840
Files:
clang/include/clang/Basic/SourceLocation.h
clang/include/clang/Edit/EditedSource.h
clang/include/clang/Tooling/Syntax/Tokens.h
clang/lib/ARCMigrate/TransGCAttrs.cpp
clang/lib/ARCMigrate/TransProperties.cpp
clang/lib/ARCMigrate/Transforms.h
clang/lib/CodeGen/CGOpenMPRuntime.cpp
clang/lib/CodeGen/CGOpenMPRuntime.h
clang/lib/Edit/EditedSource.cpp
clang/lib/Frontend/Rewrite/InclusionRewriter.cpp
clang/lib/Tooling/Syntax/Tokens.cpp
Index: clang/lib/Tooling/Syntax/Tokens.cpp
===
--- clang/lib/Tooling/Syntax/Tokens.cpp
+++ clang/lib/Tooling/Syntax/Tokens.cpp
@@ -297,7 +297,7 @@
Collector->PP.getSourceManager().isBeforeInTranslationUnit(
Range.getBegin(), LastExpansionEnd)))
return;
-Collector->Expansions[Range.getBegin().getRawEncoding()] = Range.getEnd();
+Collector->Expansions[Range.getBegin()] = Range.getEnd();
LastExpansionEnd = Range.getEnd();
}
// FIXME: handle directives like #pragma, #include, etc.
@@ -524,7 +524,7 @@
auto L = File.SpelledTokens[NextSpelled].location();
if (Offset <= SM.getFileOffset(L))
return llvm::None; // reached the offset we are looking for.
- auto Mapping = CollectedExpansions.find(L.getRawEncoding());
+ auto Mapping = CollectedExpansions.find(L);
if (Mapping != CollectedExpansions.end())
return Mapping->second; // found a mapping before the offset.
}
Index: clang/lib/Frontend/Rewrite/InclusionRewriter.cpp
===
--- clang/lib/Frontend/Rewrite/InclusionRewriter.cpp
+++ clang/lib/Frontend/Rewrite/InclusionRewriter.cpp
@@ -44,13 +44,13 @@
bool ShowLineMarkers; ///< Show #line markers.
bool UseLineDirectives; ///< Use of line directives or line markers.
/// Tracks where inclusions that change the file are found.
- std::map FileIncludes;
+ std::map FileIncludes;
/// Tracks where inclusions that import modules are found.
- std::map ModuleIncludes;
+ std::map ModuleIncludes;
/// Tracks where inclusions that enter modules (in a module build) are found.
- std::map ModuleEntryIncludes;
+ std::map ModuleEntryIncludes;
/// Tracks where #if and #elif directives get evaluated and whether to true.
- std::map IfConditions;
+ std::map IfConditions;
/// Used transitively for building up the FileIncludes mapping over the
/// various \c PPCallbacks callbacks.
SourceLocation LastInclusionLocation;
@@ -65,7 +65,7 @@
void detectMainFileEOL();
void handleModuleBegin(Token ) {
assert(Tok.getKind() == tok::annot_module_begin);
-ModuleEntryIncludes.insert({Tok.getLocation().getRawEncoding(),
+ModuleEntryIncludes.insert({Tok.getLocation(),
(Module *)Tok.getAnnotationValue()});
}
private:
@@ -164,7 +164,7 @@
return;
FileID Id = FullSourceLoc(Loc, SM).getFileID();
auto P = FileIncludes.insert(
- std::make_pair(LastInclusionLocation.getRawEncoding(),
+ std::make_pair(LastInclusionLocation,
IncludedFile(Id, NewFileType, PP.GetCurDirLookup(;
(void)P;
assert(P.second && "Unexpected revisitation of the same include directive");
@@ -199,8 +199,7 @@
const Module *Imported,
SrcMgr::CharacteristicKind FileType){
if (Imported) {
-auto P = ModuleIncludes.insert(
-std::make_pair(HashLoc.getRawEncoding(), Imported));
+auto P = ModuleIncludes.insert(std::make_pair(HashLoc, Imported));
(void)P;
assert(P.second && "Unexpected revisitation of the same include directive");
} else
@@ -209,8 +208,7 @@
void InclusionRewriter::If(SourceLocation Loc, SourceRange ConditionRange,
ConditionValueKind ConditionValue) {
- auto P = IfConditions.insert(
- std::make_pair(Loc.getRawEncoding(), ConditionValue == CVK_True));
+ auto P = IfConditions.insert(std::make_pair(Loc, ConditionValue == CVK_True));
(void)P;
assert(P.second && "Unexpected revisitation of the same if directive");
}
@@ -218,8 +216,7 @@
