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

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVObject.h:188
+  }
+  LVObject &operator=(LVObject const &) = delete;
+  virtual ~LVObject() = default;
----------------
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);
  ..
}
```



================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h:109
+  StringRef getFilename() const { return InputFilename; }
+  void setFilename(std::string Name) { InputFilename = Name; }
+  StringRef getFileFormatName() const { return FileFormatName; }
----------------
psamolysov wrote:
> probinson wrote:
> > psamolysov wrote:
> > > 
> > Should Name be a reference parameter? Passing std::string by value could result in a creating/destroying a temp copy.
> Using `std::move` after getting a parameter by value has the following advantage: if the actual parameter (`Name` in our case) is a temporary or also moved value (`rvalue`?) so:
> 
> ```
> LVReader r;
> r.setFilename(getFilename(...));
> ```
> 
> or 
> 
> ```
> LVReader r;
> std::string name = getFilename(...);
> r.setFilename(std::move(name));
> ```
> 
> the move constructor of `std::string` will be called twice and the copy constructor will not be called at all. If we use pass-by-const-reference for the `Name` parameter in the `setFilename` method, the copy constructor will be called but only once. In the general case, move constructor is much cheaper and two calls of a move constructor could be more preferable than even a single call of a copy constructor but for `std::string` this is not obvious due to short string optimization. If filenames are usually short strings, shorter than 32 characters, using pass by reference for the `Name` looks better for me.
Thanks for both great explanations.

Looking at the whole code `setFilename` is only used in one the unit tests. Replaced with `std::move`.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:211
+  virtual LVScope *getCompileUnitParent() const override {
+    return LVElement::getCompileUnitParent();
+  }
----------------
psamolysov wrote:
> I'm not sure but is `using LVElement::getCompileUnitParent();` not enough?
Sorry, I'm afraid I don't follow you.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:214
+bool LVScope::removeElement(LVElement *Element) {
+  auto predicate = [&](LVElement *Item) -> bool { return Item == Element; };
+  auto removeElement = [&](auto &Container) -> bool {
----------------
psamolysov wrote:
> `Element` is just a pointer, it can be captured by value.
Capture `Element` by value.
```
  auto Predicate = [Element](LVElement *Item) -> bool { return Item == Element; };
```


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:215
+  auto predicate = [&](LVElement *Item) -> bool { return Item == Element; };
+  auto removeElement = [&](auto &Container) -> bool {
+    auto Iter = std::remove_if(Container->begin(), Container->end(), predicate);
----------------
psamolysov wrote:
> Only captured variables are `Element` that is a pointer and `predicate` that is a lambda with a very short capture list. Both can be captured by value but by reference is ok too.
Capture both `Element` and `Predicate` by value.
```
auto RemoveElement = [Element, Predicate](auto &Container) -> bool {
```


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:254-255
+  LVSymbols References;
+  References.insert(References.end(), ReferenceSymbols->begin(),
+                    ReferenceSymbols->end());
+
----------------
psamolysov wrote:
> 
Replaced with `append`.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:260
+        std::remove_if(Symbols.begin(), Symbols.end(),
+                       [&](LVSymbol *Item) -> bool { return Item == Symbol; });
+    if (Iter != Symbols.end())
----------------
psamolysov wrote:
> `Symbol` is just a pointer and can be captured by value.
Capture `Symbol` by value.
```
[Symbol](LVSymbol *Item) -> bool { return Item == Symbol; });
```


================
Comment at: llvm/unittests/DebugInfo/LogicalView/LogicalElementsTest.cpp:64
+  template <typename T> T *create() {
+    T *Element = new T();
+    EXPECT_NE(Element, nullptr);
----------------
psamolysov wrote:
> A `std::bad_alloc` will be thrown if the test is out of memory. It makes sense to wrap the line in the `try-catch` block or use the non-throwing overload of the new operator.
Changed to use the non-throwing overload.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125778