[PATCH] D122920: [Clang][CodeGen]Fix __builtin_dump_struct missing record type field name

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I don't particularly get what the 'fix' is here to Richard's issue.  Additionally, I think this is showing me the absolute desperate need we have for some sort of 'text-checking' tests for this feature, checking vs the IR is too unreadable.



================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2048
                                LValue RecordLV, CharUnits Align,
-                               llvm::FunctionCallee Func, int Lvl) {
+                               bool dumpTypeName, llvm::FunctionCallee Func,
+                               int Lvl) {
----------------
Not a fan of bool parameters, they make calls harder to read.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2132
+
+      // If current field is a record type, we should not dump the type name in
+      // recursive dumpRecord call.
----------------
This comment isn't clear to me, can you clarify?  I thought the issue that @rsmith reported was that we dumped too types rarely, but it looks like you're taking cases where we already dumped the typename and stopped doing so?


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2141
+      TmpRes =
+          dumpRecord(CGF, CanonicalType, FieldLV, Align, false, Func, Lvl + 1);
       Res = CGF.Builder.CreateAdd(TmpRes, Res);
----------------



================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2705
     LValue RecordLV = MakeAddrLValue(RecordPtr, Arg0Type, Arg0Align);
-    Value *Res = dumpRecord(*this, Arg0Type, RecordLV, Arg0Align,
+    Value *Res = dumpRecord(*this, Arg0Type, RecordLV, Arg0Align, true,
                             {LLVMFuncType, Func}, 0);
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122920