[PATCH] D137933: [llvm-debuginfo-analyzer] 10 - Smart Pointers
David Blaikie via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Nov 14 12:58:12 PST 2022
dblaikie added a comment.
(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?
================
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>;
----------------
Looks like these are unused, should they be removed?
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h:79
+ using LVObjectPtr = std::unique_ptr<LVObject>;
+ SmallVector<LVObjectPtr> AllocatedObjects;
+
----------------
This addition of a new owning data structure (similar to some other comments I've made) makes me a bit uncomfortable - were there existing memory ownership issues in this code that this is addressing, or was the existing ownership hard to model in C++ explicit ownership? Or some other reason this is being introduced?
================
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;
----------------
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?)
================
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() {}
----------------
Remove this, since it's restating the default behavior of the implicit dtor?
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:559-568
+ const LVTagOffsetsMap &getDebugTags() const { return DebugTags; }
+ const LVOffsetElementMap &getWarningOffsets() const { return WarningOffsets; }
+ const LVOffsetLocationsMap &getInvalidLocations() const {
return InvalidLocations;
}
- const LVOffsetSymbolMap getInvalidCoverages() const {
+ const LVOffsetSymbolMap &getInvalidCoverages() const {
return InvalidCoverages;
----------------
Are these incidental/unrelated fixes? Maybe pre-commit/post-commit them to keep this patch more focussed? (oh, I guess maybe they're motivated by this patch because without unique_ptr these types were copyable, but probably weren't intended to be copied here anyway - so precommit cleanup might be suitable?)
================
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);
----------------
Maybe simplify this?
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:157
LVDoubleMap() = default;
- ~LVDoubleMap() {
- for (auto &Entry : FirstMap)
- delete Entry.second;
- }
+ ~LVDoubleMap() {}
----------------
Remove this, since it's the default?
(& could probably remove the default ctor too, separately, since that's the default as well)
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:184
- LVSecondMapType *SecondMap = FirstIter->second;
+ LVSecondMapType *SecondMap = (FirstIter->second).get();
return SecondMap;
----------------
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSupport.h:184-185
- LVSecondMapType *SecondMap = FirstIter->second;
+ LVSecondMapType *SecondMap = (FirstIter->second).get();
return SecondMap;
}
----------------
dblaikie wrote:
>
(I'd probably skip naming a variable here, FWIW - and roll the initialization into the return)
================
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);
----------------
(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))
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVSymbol.h:76
LVSymbol &operator=(const LVSymbol &) = delete;
- ~LVSymbol() { delete Locations; }
+ ~LVSymbol() = default;
----------------
Remove this default?
================
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>;
----------------
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.
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/LVReaderHandler.h:77
LVReaderHandler &operator=(const LVReaderHandler &) = delete;
- ~LVReaderHandler() { destroyReaders(); }
+ ~LVReaderHandler() = default;
----------------
remove this default?
================
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]);
----------------
I /think/ we can write this as `return Err;` now we're using C++17?
================
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));
----------------
Perhaps?
================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Readers/LVCodeViewVisitor.h:286
llvm::pdb::InputFile &Input);
- ~LVLogicalVisitor();
+ ~LVLogicalVisitor() = default;
----------------
Remove this because it's the same as the default?
================
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));
----------------
push_back is probably fine here, since it's just calling an implicit move ctor, rather than any explicit operation?
================
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();
----------------
(or remove the named variable entirely, perhaps - and roll the expressions together into `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>();
----------------
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`?)?
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:355-366
LVRange *Range = nullptr;
// Check if we already have a mapping for this section index.
LVSectionRanges::iterator IterSection = SectionRanges.find(SectionIndex);
if (IterSection == SectionRanges.end()) {
- Range = new LVRange();
- SectionRanges.emplace(SectionIndex, Range);
+ LVRangePtr RangePtr = std::make_unique<LVRange>();
+ Range = RangePtr.get();
+ SectionRanges.emplace(SectionIndex, std::move(RangePtr));
----------------
Perhaps this'd be simpler as:
also - if this function never returns null, it should probably return by reference instead of pointer?
And if this map never contains null elements, perhaps it could contain values, instead of unique_ptrs? (they still have pointer stability in a std::map - so that probably is enough?)
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:431
+ LVLines *Instructions = InstructionsPtr.get();
+ DiscoveredLines.emplace_back(std::move(InstructionsPtr));
----------------
What's the ownership model for "Instructions"/why did `DiscoveredLines` get introduced in this patch? (was there a memory ownership issue in the raw-new-using code that this is addressing? Otherwise I Wouldn't expect to see new containers in this patch)
================
Comment at: llvm/lib/DebugInfo/LogicalView/Readers/LVBinaryReader.cpp:887
- LVLines *InlineeLines = InlineeIter->second;
+ LVLines *InlineeLines = (InlineeIter->second).get();
LLVM_DEBUG({
----------------
Probably turn this into a reference? (could do that sort of thing pre or postpatch I guess)
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