[PATCH] D32177: Using address range map to speedup finding inline stack for address.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Looks good - some optional bits & pieces. (the else-after-return should be fixed before committing, the others are just FYI in case they appeal to you)



================
Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:384-385
+    return R->second.second;
+  else
+    return DWARFDie();
 }
----------------
Avoid "else after return" ( http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return )

For short cases like this I'm always undecided about whether to put the 'failure' case early or the otehr way around. But I'd probably end up with:

  if (Address >= R->second.first)
    return DWARFDie();
  return R->second.second;


================
Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:393-399
+  DWARFDie SubroutineDIE;
   // Try to look for subprogram DIEs in the DWO file.
   parseDWO();
   if (DWO)
-    SubprogramDIE = DWO->getUnit()->getSubprogramForAddress(Address);
+    SubroutineDIE = DWO->getUnit()->getSubroutineForAddress(Address);
   else
+    SubroutineDIE = getSubroutineForAddress(Address);
----------------
& while I'm suggesting changes (again, probably best, if you think they're a good idea, to commit these separately):

  parseDWO();
  DWARFDie SubroutineDIE = (DWO ? DWO->getUnit() : this)->getSubroutineForAddress(Address);

(both reducing the scope of the SubroutineDIE variable, and removing the duplication in the if/else branches)


================
Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:408
+  } else
     InlinedChain.clear();
 }
----------------
Not related to your patch, but this 'clear()' looks suspect - since the other side of this if/else adds things to InlinedChain unconditionally (without clearing first) - it seems it must be an invariant of this function that it be called with InlinedChain empty to begin with.

If you like, you could remove this line, add an "assert(InlinedChain.empty())" at the start of the function, then you could reduce the indentation of the code you've added by using a simpler early exit:

  if (!SubroutineDIE)
    return;
  while (SubroutineDIE) {
    ...

(but could be a follow-up commit (no need to send it for review first) for clarity/separation of concerns)


https://reviews.llvm.org/D32177