[PATCH] D122822: [Clang][CodeGen]Add constant array support for __builtin_dump_sturct

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2048
+                                              QualType Type) {
+  static llvm::DenseMap<QualType, StringRef> Types;
   if (Types.empty()) {
----------------
Instead of this initialization, can we make this a constexpr constructor/collection of some sort and instantiate it inline?  


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2049
+  static llvm::DenseMap<QualType, StringRef> Types;
   if (Types.empty()) {
     Types[Context.CharTy] = "%c";
----------------
I wonder if instead of this whole business, this function should just be replaced with a 'switch' on the types.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2071
+  if (Types.find(Type) == Types.end())
+    return Types[Context.VoidPtrTy];
+  return Types[Type];
----------------
When can we hit this case?  Does this mean that any types we dont understand we are just treating as void ptrs?  


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2119
+  // But if the array length is constant, we can suppress that.
+  if (llvm::ConstantInt *CL = dyn_cast<llvm::ConstantInt>(Length)) {
+    // ...and if it's constant zero, we can just skip the entire thing.
----------------
This branch doesn't seem right?  We are in a `ConstantArrayType`, which means we DEFINITELY know the length.  Additionally, `Length` is constructed directly from a `llvm::ConstantInt::get`, so we already know this answer.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2138
+
+  if (CheckZeroLength) {
+    llvm::Value *isEmpty =
----------------
We already know if this is a 'zero' length array, we should probably just not emit IR at all in that case.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2204
+  if (inArray) {
+    GString = CGF.Builder.CreateGlobalStringPtr("{\n");
+  } else {
----------------
instead of `inArray` you probably should have something a little more descriptive here, it seems what you really need is something like, `includeTypeName`, preferably as an enum to make it more clear at the caller side.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2257
+
+    if (CanonicalType->isConstantArrayType()) {
+      GString = CGF.Builder.CreateGlobalStringPtr(
----------------
Do we want to do anything for the OTHER array types?  


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2827
     LValue RecordLV = MakeAddrLValue(RecordPtr, Arg0Type, Arg0Align);
-    Value *Res = dumpRecord(*this, Arg0Type, RecordLV, Arg0Align,
+    Value *Res = dumpRecord(*this, Arg0Type, RecordLV, Arg0Align, false,
                             {LLVMFuncType, Func}, 0);
----------------
Typically we do a comment to explain what bool parms mean.  Alternatively, if instead you could create an enum of some sort that would better describe the 'false', it wouldlikely be more helpful.  (same on 2252).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122822