[PATCH] D125778: [llvm-debuginfo-analyzer] 03 - Logical elements

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:922
+
+void LVScopeCompileUnit::printScopeSize(LVScope *Scope, raw_ostream &OS) {
+  LVSizesMap::iterator Iter = Sizes.find(Scope);
----------------
probinson wrote:
> Could this be `const LVScope *Scope` ? Which would avoid some casting in printSizes.
Changing to `const LVScope *Scope` and with additional couple minor changes, removed the casting in `printSizes`.



================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:947
+  // Recursively print the contributions for each scope.
+  std::function<void(LVScope * Scope)> PrintScope = [&](LVScope *Scope) {
+    if (Scope->getLevel() < options().getOutputLevel()) {
----------------
probinson wrote:
> Is there a reason this isn't a lambda?
> 
> Also, if the parameter is const then some of the casting below can go away.
I had compiling issues while trying to use a recursive lambda.
Changing to `const LVScope *Scope` the castings are eliminated. 


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVScope.cpp:969
+  printScopeSize(const_cast<LVScopeCompileUnit *>(this), OS);
+  PrintScope(const_cast<LVScopeCompileUnit *>(this));
+
----------------
probinson wrote:
> Comments above would allow removing these const_cast calls.
Done.


================
Comment at: llvm/lib/DebugInfo/LogicalView/Core/LVSupport.cpp:54
+  std::replace(Name.begin(), Name.end(), '"', '_');
+  std::replace(Name.begin(), Name.end(), ' ', '_');
+  return Name;
----------------
probinson wrote:
> While this is easy to understand, it's iterating the string 13 times. Maybe a lambda with a switch statement, and one call to `transform` ?  Or a lambda that calls strpbrk.
Good finding.
Replaced with a call to `transform` and a call to `strpbrk`.


```
// Convert the given 'Path' to lowercase and change any matching character
// from 'CharSet' into '_'.
// CharSet:
//   '/', '\', '<', '>', '.', ':', '%', '*', '?', '|', '"', ' '.
std::string llvm::logicalview::flattenedFilePath(StringRef Path) {
  std::string Name(Path);
  std::transform(Name.begin(), Name.end(), Name.begin(), tolower);

  const char *CharSet = "/\\<>.:%*?|\" ";
  char *Input = Name.data();
  while (Input && *Input) {
    Input = strpbrk(Input, CharSet);
    if (Input)
      *Input++ = '_';
  };
  return Name;
}

```


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

https://reviews.llvm.org/D125778