[PATCH] D96045: [llvm-dwarfdump][locstats] Unify handling of inlined vars with no loc

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

Looking good; a few comments inline, thanks for pushing forwards.



================
Comment at: llvm/tools/llvm-dwarfdump/Statistics.cpp:456-458
+        // This means that the DW_AT_inline fn copy is out of order,
+        // so this inlined instance will be processed later.
+        InlinedFnsToBeProcessed.push_back(Die.getOffset());
----------------
Should this be setting InlinedVarsPtr to nullptr? Inlined variables from a higher-up-the-stack inlining site might still be present and pointed to by the InlinedVarsPtr argument.


================
Comment at: llvm/tools/llvm-dwarfdump/Statistics.cpp:653-655
+          InlinedVars.erase(std::remove(InlinedVars.begin(), InlinedVars.end(),
+                                        (*OffsetVar).getRawUValue()),
+                            InlinedVars.end());
----------------
Isn't this "removing" twice, once with std::remove, the other with the erase method?


================
Comment at: llvm/tools/llvm-dwarfdump/Statistics.cpp:658
+
+      Child = Child.getSibling();
+    }
----------------
Hmmmmm, this isn't going to collect variables that are nested within lexical_block's I guess, and fixing that probably requires more recursion. Ouch.


================
Comment at: llvm/tools/llvm-dwarfdump/Statistics.cpp:713
   /// version.
   unsigned Version = 6;
   unsigned VarParamTotal = 0;
----------------
I reckon we'll need to bump the version number for this.


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

https://reviews.llvm.org/D96045