[PATCH] D125778: [llvm-dva] 03 - Logical elements

Pavel Samolysov via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 29 01:50:05 PDT 2022


psamolysov added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h:188
+  }
+  LVObject &operator=(LVObject const &) = delete;
+  virtual ~LVObject() = default;
----------------
CarlosAlbertoEnciso wrote:
> psamolysov wrote:
> > We have a copy constructor but no copy assigned operator. Could you explain why? Thank you.
> Good question. We do not allow any copy/assignment of logical elements.
> 
> The only place where we allow copy an LVObject is in `printAttributes` when we need to print some modified attributes for the current object. Those modifications are temporary and are used to create a fake scope.
> ```
> void LVObject::printAttributes(raw_ostream &OS, bool Full, StringRef Name,
>                                LVObject *Parent, StringRef Value,
>                                bool UseQuotes, bool PrintRef) const {
>   // The current object will be the enclosing scope, use its offset and level.
>   LVObject Object(*Parent);
>   Object.setLevel(Parent->getLevel() + 1);
>   Object.setLineNumber(0);
>   Object.printAttributes(OS, Full);
>   ..
> }
> ```
> 
In this case, the copy constructor and assignment operator could be `protected`. Thank you for the explanation why the copy constructor is required but assignment operator is not.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:211
+  virtual LVScope *getCompileUnitParent() const override {
+    return LVElement::getCompileUnitParent();
+  }
----------------
CarlosAlbertoEnciso wrote:
> psamolysov wrote:
> > I'm not sure but is `using LVElement::getCompileUnitParent();` not enough?
> Sorry, I'm afraid I don't follow you.
`LVElement` is a base class for `LVScope` and it already has the public method `getCompileUnitParent`. I cannot get why you override this method in the `LVScope` class if this just delegates a call into the method of the basic class? I guessed this is due some overload resolution problems and it could be better just to inject the `getCompileUnitParent` name into the scope using the `using` keyword:

```
using LVElement::getCompileUnitParent;
```
(sorry, `()` after the function's name is not needed).

But if you aren't going to change the logic of the `LVScope::getCompileUnitParent` method in future patches, even `using` is not required, I think. However, if you are, I believe it could be better to override the method in the patch where you change the logic, it makes no sense to have an override method which just delegates the call to the basic one.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125778/new/

https://reviews.llvm.org/D125778



More information about the llvm-commits mailing list