[PATCH] D80197: [DebugInfo] Upgrade DISubrange to support Fortran dynamic arrays

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

I think this is pretty good. A couple of minor comments inline.



================
Comment at: llvm/include/llvm/IR/DIBuilder.h:577
+    DISubrange *getOrCreateSubrange(Metadata *Count, Metadata *LowerBound,
+                                    Metadata *UpperBound, Metadata *Stride);
 
----------------
Why does this take a `Metadata *` over a `DIExpression *` or something else more specific?
Update: The PointerUnion<> that is BoundType?


================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:355
+    if (!getRawLowerBound())
+      return BoundType();
+
----------------
I think it might be reasonable to assert() that the type is well-formed here.


================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:364
+    if (auto *MD = dyn_cast<DIExpression>(getRawLowerBound()))
+      return BoundType(MD);
+
----------------
Can you move these function bodies into the .cpp file?


================
Comment at: llvm/lib/IR/Verifier.cpp:913
+  AssertDI(!N.getRawStride() || N.getStride(),
+           "Stride must be signed constant or DIVariable or DIExpression", &N);
 }
----------------
In other DINodes we have getSomeAttribute() method assert when it's malformed, but the getRawAttribute() returns a raw Metadata pointer, and the Verifier checks that the Metadata node is a legal one.


================
Comment at: llvm/unittests/IR/MetadataTest.cpp:1269
+
+  EXPECT_EQ(N, DISubrange::get(Context, nullptr, LE, UE, SE));
+}
----------------
Let me know if I missed it: Is there a test for the LLVMContextImpl hashing here? Where it's tested that two DISubranges that differ in only one attribute compare differently?


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

https://reviews.llvm.org/D80197