[PATCH] D42940: Fix a crash when emitting DIEs for variable-length arrays

[Davide Italiano via Phabricator via llvm-commits]

davide added a comment.

Some comments inline, but this is almost good to go IMHO.



================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:573-583
+  if (auto *Array = dyn_cast<DICompositeType>(A->getType())) {
+    for (auto *El : Array->getElements()) {
+      if (auto *Subrange = dyn_cast<DISubrange>(El)) {
+        auto Count = Subrange->getCount();
+        if (auto *Var = Count.dyn_cast<DIVariable *>())
+          return Var == B->getVariable();
+        return false;
----------------
I think the logic here is correct, but I don't necessarily like all this level of nesting.
Can you try to use something like `llvm::any_of` or reduce indentations with early exits?
i.e.

```
DICompositeType *Array = dyn_cast<>();
if (!Array)
  return false
[...]
```


================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:586
+
+/// Sort local variables so that variables that variables that appear inside of
+/// helper expressions come first.
----------------
`that variables that variables` :) 


================
Comment at: lib/CodeGen/AsmPrinter/DwarfFile.h:52
+  struct ScopeVars {
+    std::map<unsigned, DbgVariable *> Args;
+    SmallVector<DbgVariable *, 8> Locals;
----------------
aprantl wrote:
> davide wrote:
> > Do you really need `std::map<>` here? Can't you use an LLVM container?
> Is there a better container for this? I need something that allows me to
> 1. check whether an element already exists while building the list
> 2. traverse all elements in order at the end.
> 
> DenseMap doesn't support (2), so the only other alternative I could come up with was a SmallVector or a std::list that is kept sorted after each insert by using std::lower_bound to look up the insertion point. Using a std::list instead the SmallVector would be more memory efficient than the std::map. Using a SmallVector would use least memory, but has quadratic worst-case behavior. Any preferences/suggestions?
I think a `map` is fine, but I'd add a comment here explaining the rationale, if you don't mind.


https://reviews.llvm.org/D42940