[PATCH] D137933: [llvm-debuginfo-analyzer] 10 - Smart Pointers
Carlos Alberto Enciso via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 15 06:58:05 PST 2022
CarlosAlbertoEnciso added a comment.
In D137933#3925820 <https://reviews.llvm.org/D137933#3925820>, @dblaikie wrote:
> (some suggestions here might be too noisy to do all in this review - maybe a matter of splitting things up - the discussion about new data structures/changes in ownership might be one of those worth breaking down into smaller steps, not sure)
>
> Perhaps some general discussion about the ownership/lifetime model of everything here would be useful to add to the patch description to get a general lay of the land?
First of all, thanks very much for your reviews.
I have addressed some of them. Tomorrow I will finish the remaining (including a general discussion about the ownership/lifetime) and upload a new patch.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h:84-90
+using LVElementPtr = std::unique_ptr<LVElement>;
+using LVLinePtr = std::unique_ptr<LVLine>;
+using LVLocationPtr = std::unique_ptr<LVLocation>;
+using LVScopePtr = std::unique_ptr<LVScope>;
+using LVSymbolPtr = std::unique_ptr<LVSymbol>;
+using LVTypePtr = std::unique_ptr<LVType>;
----------------
dblaikie wrote:
> Looks like these are unused, should they be removed?
Good finding. They are unused. Removed
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:119-132
// Types, Symbols, Scopes, Lines, Locations in this scope.
- LVAutoTypes *Types = nullptr;
- LVAutoSymbols *Symbols = nullptr;
- LVAutoScopes *Scopes = nullptr;
- LVAutoLines *Lines = nullptr;
- LVAutoLocations *Ranges = nullptr;
+ LVTypesPtr Types = nullptr;
+ LVSymbolsPtr Symbols = nullptr;
+ LVScopesPtr Scopes = nullptr;
+ LVLinesPtr Lines = nullptr;
+ LVLocationsPtr Ranges = nullptr;
----------------
dblaikie wrote:
> Don't need the `= nullptr` for std::unique_ptr, that's the default behavior of the default ctor. (this is one of the reasons I'm a bit reticent to use aliases for unique_ptr names - I'm not sure the name shortening is worth the benefit compared to the obfuscation of these sort of core types with known semantics - might be better to keep the explicit `unique_ptr` usage here/generally?)
Happy to remove all the aliases for `std::unique_ptr` and keep the explicit `unique_ptr`.
`std::unique_ptr<LVElements> Children;`
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:495
LVScopeCompileUnit &operator=(const LVScopeCompileUnit &) = delete;
- ~LVScopeCompileUnit() {
- deleteList<LVTagOffsetsMap>(DebugTags);
- deleteList<LVOffsetLocationsMap>(InvalidLocations);
- deleteList<LVOffsetLocationsMap>(InvalidRanges);
- deleteList<LVOffsetLinesMap>(LinesZero);
- }
+ ~LVScopeCompileUnit() {}
----------------
dblaikie wrote:
> Remove this, since it's restating the default behavior of the implicit dtor?
Changed to `~LVScopeCompileUnit() = default;` just for consistency with other `LVScopexxx` definitions.
May be in a post-commit: remove all default dtors.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:135-139
+ typename MapType::iterator Iter = Map->find(Key);
+ if (Iter == Map->end())
+ Map->emplace(Key, std::initializer_list<ValueType>{Value});
+ else
+ (*Iter).second.push_back(Value);
----------------
dblaikie wrote:
> Maybe simplify this?
Nice simplification. Changed to
`(*Map)[Key].push_back(Value);`
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:157
LVDoubleMap() = default;
- ~LVDoubleMap() {
- for (auto &Entry : FirstMap)
- delete Entry.second;
- }
+ ~LVDoubleMap() {}
----------------
dblaikie wrote:
> Remove this, since it's the default?
> (& could probably remove the default ctor too, separately, since that's the default as well)
Removed both default ctor and dtor.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:184
- LVSecondMapType *SecondMap = FirstIter->second;
+ LVSecondMapType *SecondMap = (FirstIter->second).get();
return SecondMap;
----------------
dblaikie wrote:
> dblaikie wrote:
> >
> (I'd probably skip naming a variable here, FWIW - and roll the initialization into the return)
Removed the extra parenthesis.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:184-185
- LVSecondMapType *SecondMap = FirstIter->second;
+ LVSecondMapType *SecondMap = (FirstIter->second).get();
return SecondMap;
}
----------------
CarlosAlbertoEnciso wrote:
> dblaikie wrote:
> > dblaikie wrote:
> > >
> > (I'd probably skip naming a variable here, FWIW - and roll the initialization into the return)
> Removed the extra parenthesis.
Removed the extra variable.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:211-212
for (typename LVFirstMapType::const_reference FirstEntry : FirstMap) {
- LVSecondMapType *SecondMap = FirstEntry.second;
+ LVSecondMapType *SecondMap = (FirstEntry.second).get();
for (typename LVSecondMapType::const_reference SecondEntry : *SecondMap)
Values.push_back(SecondEntry.second);
----------------
dblaikie wrote:
> (or otherwise avoid the need to call `.get()` on one line only to dereference on the next (eg: if you find the named variable important for readability, change its type to a reference - and do `type &x = *y; for (... : x)` instead of .get()+*, perhaps))
Removed extra parenthesis.
Keep named variable for readability.
Changed to
```
for (typename LVFirstMapType::const_reference FirstEntry : FirstMap) {
LVSecondMapType &SecondMap = *FirstEntry.second;
for (typename LVSecondMapType::const_reference SecondEntry : SecondMap)
Values.push_back(SecondEntry.second);
}
```
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/LVReaderHandler.h:30
-using LVReaders = std::vector<LVReader *>;
+using LVReaderObj = std::unique_ptr<LVReader>;
+using LVReaders = std::vector<LVReaderObj>;
----------------
dblaikie wrote:
> If there are going to be aliases for unique_ptr types, maybe worth using a consistent suffix (this one is Obj, others are Ptr) - and maybe SP rather than Ptr, to make it clear it's a smart pointer? But generally I'd avoid the aliases & find that seeing unique_ptr is probably sufficiently more clear to readers so as to be worth the extra characters required.
Happy to remove all the aliases for `std::unique_ptr` and keep the explicit `unique_ptr`.
`using LVReaders = std::vector<std::unique_ptr<LVReader>>`;
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/LVReaderHandler.h:77
LVReaderHandler &operator=(const LVReaderHandler &) = delete;
- ~LVReaderHandler() { destroyReaders(); }
+ ~LVReaderHandler() = default;
----------------
dblaikie wrote:
> remove this default?
Removed.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/LVReaderHandler.h:87
if (Error Err = createReader(Pathname, Readers))
return std::move(Err);
+ return std::move(Readers[0]);
----------------
dblaikie wrote:
> I /think/ we can write this as `return Err;` now we're using C++17?
You are correct. Changed to `return Err;`.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVBinaryReader.h:167-170
+ LVLinesPtr InlineeLinesPtr = std::make_unique<LVLines>();
+ LVLines *InlineeLines = InlineeLinesPtr.get();
*InlineeLines = std::move(Lines);
+ CUInlineeLines.emplace(Scope, std::move(InlineeLinesPtr));
----------------
dblaikie wrote:
> Perhaps?
Nice simplification.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.h:286
llvm::pdb::InputFile &Input);
- ~LVLogicalVisitor();
+ ~LVLogicalVisitor() = default;
----------------
dblaikie wrote:
> Remove this because it's the same as the default?
Removed.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVLocation.cpp:583
+ Entries = std::make_unique<LVOperationSet>();
+ Entries->emplace_back(
+ std::make_unique<LVOperation>(Opcode, Operand1, Operand2));
----------------
dblaikie wrote:
> push_back is probably fine here, since it's just calling an implicit move ctor, rather than any explicit operation?
Good point. Changed to `push_back`.
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVLocation.cpp:610-611
if (Entries && Entries->size() == 1) {
- LVOperation *Operation = Entries->front();
+ LVOperation *Operation = (Entries->front()).get();
if (dwarf::DW_OP_fbreg == Operation->getOpcode())
setIsStackOffset();
----------------
dblaikie wrote:
> (or remove the named variable entirely, perhaps - and roll the expressions together into `Entries->front()->getOpcode()`)
Nice simplification. Changed to
`if (dwarf::DW_OP_fbreg == Entries->front()->getOpcode())`
================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:130-131
assert(!Line->getParent() && "Line already inserted");
if (!Lines)
- Lines = new LVAutoLines();
+ Lines = std::make_unique<LVLines>();
----------------
dblaikie wrote:
> There's a lot of code like this - `Lines` here, `Children` above, `Ranges` and `Scopes` below, etc - are they all needed, or could some of them be turned into direct values (eg: `LVLines Lines` rather than `unique_ptr<LVLines> Lines`?)?
This is a good question. They are all needed.
The reason to define `Lines`, `Children`, `Ranges`, `Scopes`, etc in that way is to keep the size of `LVScope` as small as possible.
```
struct LVLines {};
class ClassPointer {
std::unique_ptr<SmallVector<LVLines *>> Lines;
};
class ClassVector {
SmallVector<LVLines *,2> Lines;
};
```
ClassPointer size = 8 bytes
ClassVector size = 32 bytes
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:887
- LVLines *InlineeLines = InlineeIter->second;
+ LVLines *InlineeLines = (InlineeIter->second).get();
LLVM_DEBUG({
----------------
dblaikie wrote:
> Probably turn this into a reference? (could do that sort of thing pre or postpatch I guess)
Removed the extra parenthesis.
Changing `LVLines *InlineeLines` to `reference` can be done as a postpatch.
Adding
```
// TODO: Convert this into a reference.
LVLines *InlineeLines = InlineeIter->second.get();
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137933/new/
https://reviews.llvm.org/D137933
More information about the llvm-commits
mailing list