[Lldb-commits] [PATCH] D119178: Add support for generating debug-info for structured bindings of structs and arrays

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added inline comments.


================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:4667
+
+  SmallVector<uint64_t, 13> Expr;
+  AppendAddressSpaceXDeref(AddressSpace, Expr);
----------------
aprantl wrote:
> 13 seems to be unnecessarily high. Shouldn't 1 be enough for the expected single DW_OP_deref?
Good catch, I used the `VarDecl` version of `EmitDeclare` as an example and I copied it from there.

I will also be pushing the `DW_OP_plus_uconst` and the value later on as well. So maybe 3 is a the right value.


================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:4713
+                           .toCharUnitsFromBits(value * typeSize)
+                           .getQuantity());
+      }
----------------
aprantl wrote:
> Can C++ arrays ever have a non-zero stride? (perhaps due to element alignment?)
Elements must be contiguous and start at `0` see [dcl.array/p6](http://eel.is/c++draft/dcl.array#6):

>An object of type “array of N U” consists of a contiguously allocated non-empty set of N subobjects of type U, known as the elements of the array, and numbered 0 to N-1.

The size of the type should include any padding. 


================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:4716
+    }
+  }
+
----------------
aprantl wrote:
> Should there be an else { assert("unhandled binding expressions"); } here or are there other expressions that just don't need special handling?
We should only have three cases:

- struct
- array
- tuple-like 

We exit early for the tuple-like case at the top b/c we handle that elsewhere. 

So maybe asserting we never end up with a not handled case may make sense.


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

https://reviews.llvm.org/D119178