[PATCH] D21526: [codeview] Improved array type support (multi dimension array)

[Reid Kleckner via llvm-commits]

rnk added a comment.

Thanks, seems like the right approach.


================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:858-862
@@ +857,7 @@
+
+  // FIXME:
+  // There is a bug in the front-end where an array of an incomplete structure
+  // declaration ends up not getting a size assigned to it.  This needs to
+  // be fixed in the front-end, but in the meantime we don't want to trigger an
+  // assertion because of this.
+  if (Ty->getSizeInBits() == 0) {
----------------
This shouldn't be a FIXME. There is nothing we can do in the frontend when the type definition is unavailable, as in the example you used in your test case. MSVC gives the array size zero, which is what your code does. You should reword the comment to document that this is intended behavior.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:876
@@ +875,3 @@
+    int64_t LowerBound = Subrange->getLowerBound();
+    int64_t DefaultLowerBound = 0; // FIXME : default bound
+    int64_t Count = Subrange->getCount();
----------------
Why not just `assert(Subrange->getLowerBound() == 0 && "codeview doesn't support subranges with lower bounds");`? As is, your code will probably trigger unused variable warnings in NDEBUG builds.

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:881
@@ +880,3 @@
+
+    // FIXME: this is a WA solution until solving dynamic array boundary.
+    if (Count == -1) {
----------------
This comment needs work. It's worth mentioning that a count of -1 means that this is a VLA, and that it is not currently representable in CodeView. Actually, why not just let it go through as ~0U?

================
Comment at: lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:894
@@ +893,3 @@
+
+  assert(UndefinedSubrange || ElementSize == Size);
+
----------------
I think without adding `(void)UndefinedSubrange` outside the assert, you will get -Wunused-but-set warnings in an NDEBUG build.


http://reviews.llvm.org/D21526