[PATCH] D94599: [clang][Tooling] Get rid of a hack in SymbolOccurrences, NFCI
miyuki created this revision.
miyuki added reviewers: aprantl, dexonsmith, arphaman.
miyuki requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
The class `SymbolOccurrences` can store either a single `SourceRange`
in-place or multiple `SourceRanges` on the heap. In the latter case
the number of source ranges is stored in the internal representation
of the beginning `SourceLocation` of the in-place `SourceRange`
object.
This change gets rid of such hack by placing `SourceRange` in a union
which holds either a valid `SourceRange` or an `unsigned int` (a number
of ranges).
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D94599
Files:
clang/include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h
clang/lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
Index: clang/lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
===
--- clang/lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
+++ clang/lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
@@ -21,13 +21,12 @@
"mismatching number of locations and lengths");
assert(!Locations.empty() && "no locations");
if (Locations.size() == 1) {
-RangeOrNumRanges = SourceRange(
+new (&SingleRange) SourceRange(
Locations[0], Locations[0].getLocWithOffset(NamePieces[0].size()));
return;
}
MultipleRanges = std::make_unique(Locations.size());
- RangeOrNumRanges.setBegin(
- SourceLocation::getFromRawEncoding(Locations.size()));
+ NumRanges = Locations.size();
for (const auto &Loc : llvm::enumerate(Locations)) {
MultipleRanges[Loc.index()] = SourceRange(
Loc.value(),
Index: clang/include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h
===
--- clang/include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h
+++ clang/include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h
@@ -69,17 +69,18 @@
OccurrenceKind getKind() const { return Kind; }
ArrayRef getNameRanges() const {
-if (MultipleRanges) {
- return llvm::makeArrayRef(MultipleRanges.get(),
-RangeOrNumRanges.getBegin().getRawEncoding());
-}
-return RangeOrNumRanges;
+if (MultipleRanges)
+ return llvm::makeArrayRef(MultipleRanges.get(), NumRanges);
+return SingleRange;
}
private:
OccurrenceKind Kind;
std::unique_ptr MultipleRanges;
- SourceRange RangeOrNumRanges;
+ union {
+SourceRange SingleRange;
+unsigned NumRanges;
+ };
};
using SymbolOccurrences = std::vector;
Index: clang/lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
===
--- clang/lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
+++ clang/lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
@@ -21,13 +21,12 @@
"mismatching number of locations and lengths");
assert(!Locations.empty() && "no locations");
if (Locations.size() == 1) {
-RangeOrNumRanges = SourceRange(
+new (&SingleRange) SourceRange(
Locations[0], Locations[0].getLocWithOffset(NamePieces[0].size()));
return;
}
MultipleRanges = std::make_unique(Locations.size());
- RangeOrNumRanges.setBegin(
- SourceLocation::getFromRawEncoding(Locations.size()));
+ NumRanges = Locations.size();
for (const auto &Loc : llvm::enumerate(Locations)) {
MultipleRanges[Loc.index()] = SourceRange(
Loc.value(),
Index: clang/include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h
===
--- clang/include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h
+++ clang/include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h
@@ -69,17 +69,18 @@
OccurrenceKind getKind() const { return Kind; }
ArrayRef getNameRanges() const {
-if (MultipleRanges) {
- return llvm::makeArrayRef(MultipleRanges.get(),
-RangeOrNumRanges.getBegin().getRawEncoding());
-}
-return RangeOrNumRanges;
+if (MultipleRanges)
+ return llvm::makeArrayRef(MultipleRanges.get(), NumRanges);
+return SingleRange;
}
private:
OccurrenceKind Kind;
std::unique_ptr MultipleRanges;
- SourceRange RangeOrNumRanges;
+ union {
+SourceRange SingleRange;
+unsigned NumRanges;
+ };
};
using SymbolOccurrences = std::vector;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D94599: [clang][Tooling] Get rid of a hack in SymbolOccurrences, NFCI
miyuki added a comment. I will wait until the end of this week to see if anyone has objections. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94599/new/ https://reviews.llvm.org/D94599 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D94599: [clang][Tooling] Get rid of a hack in SymbolOccurrences, NFCI
simon_tatham added inline comments.
Comment at:
clang/include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h:81
+ union {
+SourceRange SingleRange;
+unsigned NumRanges;
This surely relies on `SourceRange` having no destructor (or rather, a trivial
one). If that ever changes, then destruction of this class will risk either a
spurious call to the destructor or a missing one.
Is there any way to somehow arrange that a build failure will occur if the
definition of `SourceRange` changes in that way?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D94599/new/
https://reviews.llvm.org/D94599
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D94599: [clang][Tooling] Get rid of a hack in SymbolOccurrences, NFCI
simon_tatham added inline comments.
Comment at:
clang/include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h:81
+ union {
+SourceRange SingleRange;
+unsigned NumRanges;
simon_tatham wrote:
> This surely relies on `SourceRange` having no destructor (or rather, a
> trivial one). If that ever changes, then destruction of this class will risk
> either a spurious call to the destructor or a missing one.
>
> Is there any way to somehow arrange that a build failure will occur if the
> definition of `SourceRange` changes in that way?
... aha, found one. Such as a static assertion that
`std::is_trivially_destructible::value` is 1.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D94599/new/
https://reviews.llvm.org/D94599
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D94599: [clang][Tooling] Get rid of a hack in SymbolOccurrences, NFCI
miyuki updated this revision to Diff 317627.
miyuki edited the summary of this revision.
miyuki added a comment.
Added `static_assert`s that check that `SourceRange` and `SourceLocation` are
trivially destructible.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D94599/new/
https://reviews.llvm.org/D94599
Files:
clang/include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h
clang/lib/Basic/SourceLocation.cpp
clang/lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
Index: clang/lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
===
--- clang/lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
+++ clang/lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
@@ -21,13 +21,12 @@
"mismatching number of locations and lengths");
assert(!Locations.empty() && "no locations");
if (Locations.size() == 1) {
-RangeOrNumRanges = SourceRange(
+new (&SingleRange) SourceRange(
Locations[0], Locations[0].getLocWithOffset(NamePieces[0].size()));
return;
}
MultipleRanges = std::make_unique(Locations.size());
- RangeOrNumRanges.setBegin(
- SourceLocation::getFromRawEncoding(Locations.size()));
+ NumRanges = Locations.size();
for (const auto &Loc : llvm::enumerate(Locations)) {
MultipleRanges[Loc.index()] = SourceRange(
Loc.value(),
Index: clang/lib/Basic/SourceLocation.cpp
===
--- clang/lib/Basic/SourceLocation.cpp
+++ clang/lib/Basic/SourceLocation.cpp
@@ -42,6 +42,14 @@
// SourceLocation
//===--===//
+static_assert(std::is_trivially_destructible::value,
+ "SourceLocation must be trivially destructible because it is "
+ "used in unions");
+
+static_assert(std::is_trivially_destructible::value,
+ "SourceRange must be trivially destructible because it is "
+ "used in unions");
+
unsigned SourceLocation::getHashValue() const {
return llvm::DenseMapInfo::getHashValue(ID);
}
Index: clang/include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h
===
--- clang/include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h
+++ clang/include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h
@@ -69,17 +69,18 @@
OccurrenceKind getKind() const { return Kind; }
ArrayRef getNameRanges() const {
-if (MultipleRanges) {
- return llvm::makeArrayRef(MultipleRanges.get(),
-RangeOrNumRanges.getBegin().getRawEncoding());
-}
-return RangeOrNumRanges;
+if (MultipleRanges)
+ return llvm::makeArrayRef(MultipleRanges.get(), NumRanges);
+return SingleRange;
}
private:
OccurrenceKind Kind;
std::unique_ptr MultipleRanges;
- SourceRange RangeOrNumRanges;
+ union {
+SourceRange SingleRange;
+unsigned NumRanges;
+ };
};
using SymbolOccurrences = std::vector;
Index: clang/lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
===
--- clang/lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
+++ clang/lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
@@ -21,13 +21,12 @@
"mismatching number of locations and lengths");
assert(!Locations.empty() && "no locations");
if (Locations.size() == 1) {
-RangeOrNumRanges = SourceRange(
+new (&SingleRange) SourceRange(
Locations[0], Locations[0].getLocWithOffset(NamePieces[0].size()));
return;
}
MultipleRanges = std::make_unique(Locations.size());
- RangeOrNumRanges.setBegin(
- SourceLocation::getFromRawEncoding(Locations.size()));
+ NumRanges = Locations.size();
for (const auto &Loc : llvm::enumerate(Locations)) {
MultipleRanges[Loc.index()] = SourceRange(
Loc.value(),
Index: clang/lib/Basic/SourceLocation.cpp
===
--- clang/lib/Basic/SourceLocation.cpp
+++ clang/lib/Basic/SourceLocation.cpp
@@ -42,6 +42,14 @@
// SourceLocation
//===--===//
+static_assert(std::is_trivially_destructible::value,
+ "SourceLocation must be trivially destructible because it is "
+ "used in unions");
+
+static_assert(std::is_trivially_destructible::value,
+ "SourceRange must be trivially destructible because it is "
+ "used in unions");
+
unsigned SourceLocation::getHashValue() const {
return llvm::DenseMapInfo::getHashValue(ID);
}
Index: clang/include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h
===
--- clang/include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h
+++ clang/include/clang/Tooling/Refact
[PATCH] D94599: [clang][Tooling] Get rid of a hack in SymbolOccurrences, NFCI
simon_tatham accepted this revision. simon_tatham added a comment. Thanks. LGTM now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94599/new/ https://reviews.llvm.org/D94599 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D94599: [clang][Tooling] Get rid of a hack in SymbolOccurrences, NFCI
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa0e30914f8c8: [clang][Tooling] Get rid of a hack in
SymbolOccurrences, NFCI (authored by miyuki).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D94599/new/
https://reviews.llvm.org/D94599
Files:
clang/include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h
clang/lib/Basic/SourceLocation.cpp
clang/lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
Index: clang/lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
===
--- clang/lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
+++ clang/lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
@@ -21,13 +21,12 @@
"mismatching number of locations and lengths");
assert(!Locations.empty() && "no locations");
if (Locations.size() == 1) {
-RangeOrNumRanges = SourceRange(
+new (&SingleRange) SourceRange(
Locations[0], Locations[0].getLocWithOffset(NamePieces[0].size()));
return;
}
MultipleRanges = std::make_unique(Locations.size());
- RangeOrNumRanges.setBegin(
- SourceLocation::getFromRawEncoding(Locations.size()));
+ NumRanges = Locations.size();
for (const auto &Loc : llvm::enumerate(Locations)) {
MultipleRanges[Loc.index()] = SourceRange(
Loc.value(),
Index: clang/lib/Basic/SourceLocation.cpp
===
--- clang/lib/Basic/SourceLocation.cpp
+++ clang/lib/Basic/SourceLocation.cpp
@@ -42,6 +42,14 @@
// SourceLocation
//===--===//
+static_assert(std::is_trivially_destructible::value,
+ "SourceLocation must be trivially destructible because it is "
+ "used in unions");
+
+static_assert(std::is_trivially_destructible::value,
+ "SourceRange must be trivially destructible because it is "
+ "used in unions");
+
unsigned SourceLocation::getHashValue() const {
return llvm::DenseMapInfo::getHashValue(ID);
}
Index: clang/include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h
===
--- clang/include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h
+++ clang/include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h
@@ -69,17 +69,18 @@
OccurrenceKind getKind() const { return Kind; }
ArrayRef getNameRanges() const {
-if (MultipleRanges) {
- return llvm::makeArrayRef(MultipleRanges.get(),
-RangeOrNumRanges.getBegin().getRawEncoding());
-}
-return RangeOrNumRanges;
+if (MultipleRanges)
+ return llvm::makeArrayRef(MultipleRanges.get(), NumRanges);
+return SingleRange;
}
private:
OccurrenceKind Kind;
std::unique_ptr MultipleRanges;
- SourceRange RangeOrNumRanges;
+ union {
+SourceRange SingleRange;
+unsigned NumRanges;
+ };
};
using SymbolOccurrences = std::vector;
Index: clang/lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
===
--- clang/lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
+++ clang/lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
@@ -21,13 +21,12 @@
"mismatching number of locations and lengths");
assert(!Locations.empty() && "no locations");
if (Locations.size() == 1) {
-RangeOrNumRanges = SourceRange(
+new (&SingleRange) SourceRange(
Locations[0], Locations[0].getLocWithOffset(NamePieces[0].size()));
return;
}
MultipleRanges = std::make_unique(Locations.size());
- RangeOrNumRanges.setBegin(
- SourceLocation::getFromRawEncoding(Locations.size()));
+ NumRanges = Locations.size();
for (const auto &Loc : llvm::enumerate(Locations)) {
MultipleRanges[Loc.index()] = SourceRange(
Loc.value(),
Index: clang/lib/Basic/SourceLocation.cpp
===
--- clang/lib/Basic/SourceLocation.cpp
+++ clang/lib/Basic/SourceLocation.cpp
@@ -42,6 +42,14 @@
// SourceLocation
//===--===//
+static_assert(std::is_trivially_destructible::value,
+ "SourceLocation must be trivially destructible because it is "
+ "used in unions");
+
+static_assert(std::is_trivially_destructible::value,
+ "SourceRange must be trivially destructible because it is "
+ "used in unions");
+
unsigned SourceLocation::getHashValue() const {
return llvm::DenseMapInfo::getHashValue(ID);
}
Index: clang/include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h
===
--- clang/include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h
+++ clang/include/clang/Tooling/Refactoring/Rename/SymbolOccurre
