[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