[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/LVLine.h:74
+  // String used for printing objects with no line number.
+  virtual std::string noLineAsString(bool ShowZero) const override;
+
----------------
psamolysov wrote:
> In order to be aligned with the `lineNumberAsString` method. The default value could be set up for the parameter in the other `noLineAsString` methods as well. `virtual` also can be eliminated.
Added the default value and removed `virtual`.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVReader.h:121
+  void setCompileUnit(LVScope *Scope) {
+    CompileUnit = static_cast<LVScopeCompileUnit *>(Scope);
+  }
----------------
psamolysov wrote:
> Are we sure the `Scope` are always a pointer to `LVScopeCompileUnit`? What about to add an assert in the function before the assignment?
Each LVScope, LVType, LVSymbol and LVLine have a specific bit that gives more information.
A LVScope that represents a compile unit has the following bits:

```
  KIND_3(LVScopeKind, IsCompileUnit, CanHaveRanges, CanHaveLines, TransformName);
```
Which are expanded to a series of `get/set` functions used to get specific information.

Added:

```
    assert(Scope && Scope->isCompileUnit() && "Scope is not a compile unit");
```


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:88-99
+  LVAutoTypes *Types = nullptr;
+  LVAutoSymbols *Symbols = nullptr;
+  LVAutoScopes *Scopes = nullptr;
+  LVAutoLines *Lines = nullptr;
+
+  // Vector of elements (types, scopes and symbols).
+  // It is the union of (*Types, *Symbols and *Scopes) to be used for
----------------
psamolysov wrote:
> Just a question. The `LVScope` class is not copyable at all. Why values of these `LVAutoTypes`, `LVAutoSymbols` etc. cannot be class members, not pointers to the values? At least, having the members as values eliminates a lot of `if (Children) ...` checks in the implementation.
> 
> If they cannot be just values, maybe it makes sense to use smart pointers here (`std::unique_ptr`?) instead of create the members with plain `new` and delete then in the destructor.
As the `LVScope` represents any scope, which can be a `class`, `function`, `lexical scope`, etc, in order to reduce the class size we use pointers to those `LVAutoXXXX`. For example:

```
class ClassPointer {
  void *Pointer = nullptr;
};
class ClassVector {
  SmallVector<void *,2> Vector; 
};
size_t ClassPointerSize = sizeof(ClassPointer);
size_t ClassVectorSize = sizeof(ClassVector);
```
ClassPointerSize = 8 bytes
ClassVectorSize = 32 bytes



================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:88-99
+  LVAutoTypes *Types = nullptr;
+  LVAutoSymbols *Symbols = nullptr;
+  LVAutoScopes *Scopes = nullptr;
+  LVAutoLines *Lines = nullptr;
+
+  // Vector of elements (types, scopes and symbols).
+  // It is the union of (*Types, *Symbols and *Scopes) to be used for
----------------
CarlosAlbertoEnciso wrote:
> psamolysov wrote:
> > Just a question. The `LVScope` class is not copyable at all. Why values of these `LVAutoTypes`, `LVAutoSymbols` etc. cannot be class members, not pointers to the values? At least, having the members as values eliminates a lot of `if (Children) ...` checks in the implementation.
> > 
> > If they cannot be just values, maybe it makes sense to use smart pointers here (`std::unique_ptr`?) instead of create the members with plain `new` and delete then in the destructor.
> As the `LVScope` represents any scope, which can be a `class`, `function`, `lexical scope`, etc, in order to reduce the class size we use pointers to those `LVAutoXXXX`. For example:
> 
> ```
> class ClassPointer {
>   void *Pointer = nullptr;
> };
> class ClassVector {
>   SmallVector<void *,2> Vector; 
> };
> size_t ClassPointerSize = sizeof(ClassPointer);
> size_t ClassVectorSize = sizeof(ClassVector);
> ```
> ClassPointerSize = 8 bytes
> ClassVectorSize = 32 bytes
> 
Before uploading the patches we discussed the smart pointer usage across the tool but we decided to wait until all patches are accepted. After that. the next step is to remove the new/delete machinary across the tool.


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:202
+                bool Full = true) const override;
+  void sort();
+
----------------
psamolysov wrote:
> I believe a comment is required at least to let the class' user understand that this is sort by what.
Added:

```
  // Sort the logical elements using the criteria specified by the
  // command line option '--output-sort'.

```


================
Comment at: llvm/include/llvm/DebugInfo/LogicalView/Core/LVScope.h:304
+  // Names (files and directories) used by the Compile Unit.
+  std::vector<size_t> Filenames;
+
----------------
psamolysov wrote:
> CarlosAlbertoEnciso wrote:
> > psamolysov wrote:
> > > Is this a vector of indices?
> > Yes. The vector contains indices into the String Pool table.
> > `Filenames.push_back(getStringPool().getIndex(Name));`
> Well, thank you for the answer.
Thanks.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVElement.cpp:431
+void LVElement::printFileIndex(raw_ostream &OS, bool Full) const {
+  if (options().getPrintFormatting() && options().getAttributeAnySource() &&
+      getFilenameIndex()) {
----------------
psamolysov wrote:
> CarlosAlbertoEnciso wrote:
> > psamolysov wrote:
> > > Early return if the inverted condition takes place is more preferable by the coding style.
> > I see your point.
> > 
> > ```
> > if (!(options().getPrintFormatting() && options().getAttributeAnySource() &&
> >       getFilenameIndex()))
> >   return;
> > ```
> > But with the inverted condition, it seems the statetment gets obscure.
> I see such long `if`s are actively used though the code. So, I think the code could be left as it was initially written for consistency.
Thanks


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:461
+    QualifiedName.append("::");
+  QualifiedName.append(std::string(getName()));
+}
----------------
psamolysov wrote:
> Are you sure the explicitly invoked constructor `std::string(getName())` is required? It looks like this converts a `StringRef` into a temporary `std::string`, then makes a copy of the `std::string` and put the copy into the `QualifiedName.append` method. `QualifiedName.append(getName());` should be enough, shouldn't it?
Way back the following upstream commits:
```
commit adcd02683856c30ba6f349279509acecd90063df
Author: Benjamin Kramer <benny.kra at googlemail.com>
Date:   Tue Jan 28 20:23:46 2020 +0100

    Make llvm::StringRef to std::string conversions explicit.
    
    This is how it should've been and brings it more in line with
    std::string_view. There should be no functional change here.
    
    This is mostly mechanical from a custom clang-tidy check, with a lot of
    manual fixups. It uncovers a lot of minor inefficiencies.
    
    This doesn't actually modify StringRef yet, I'll do that in a follow-up.

commit 777180a32b61070a10dd330b4f038bf24e916af1
Author: Benjamin Kramer <benny.kra at googlemail.com>
Date:   Tue Jan 28 23:30:02 2020 +0100

    [ADT] Make StringRef's std::string conversion operator explicit
    
    This has the same behavior as converting std::string_view to
    std::string. This is an expensive conversion, so explicit conversions
    are helpful for avoiding unneccessary string copies.
```

Introduced the need to use the explicit conversion.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:902
+      // Collect only unique names.
+      UniqueNames.insert(std::string(Name));
+    }
----------------
psamolysov wrote:
> It might work.
See explanation about the needed explicit conversion.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:904
+    }
+    for (std::string Name : UniqueNames)
+      OS << std::string(Indentation, ' ') << formattedKind(Kind) << " "
----------------
psamolysov wrote:
> 
Changed.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVSort.cpp:41
+                                           const LVObject *RHS) {
+  return std::string(LHS->getName()) < std::string(RHS->getName());
+}
----------------
psamolysov wrote:
> If `getName()` returns an `llvm::StringRef` then there is the `operator<` for `llvm::StringRef`, so there is no need to create a temporary `std::string` for LHS as well as `RHS`.
Very good catch. Changed to `return LHS->getName() < RHS->getName();`


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVType.cpp:264
+      // The argument types always are qualified.
+      Name.append(std::string(getTypeQualifiedName()));
+
----------------
psamolysov wrote:
> Just to try. `llvm::StringRef` should be convertible to `std::string`. If this works, the other creations of temporary `std::string` in this function will be able to be eliminated too.
See previous explanation for the needed explicit conversion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125778