[PATCH] D44093: [BUILTINS] structure pretty printer

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: lib/CodeGen/CGBuiltin.cpp:1208
+
+    assert(RT && "The first argument must be a record type");
+
----------------
paulsemel wrote:
> aaron.ballman wrote:
> > I don't see anything enforcing this constraint, so users are likely to hit this assertion rather than a compile error.
> I actually didn't manage to enforce this in the builtin declaration as we can only declare simple types (I am probably missing something)
I think this builtin will require custom type checking (so its signature needs a `t`), and then you can add your code to `Sema::CheckBuiltinFunctionCall()` to do the actual semantic checking.


================
Comment at: lib/CodeGen/CGBuiltin.cpp:1258
+
+      //Need to handle bitfield here
+
----------------
paulsemel wrote:
> aaron.ballman wrote:
> > Are you intending to implement this as part of this functionality?
> Yes, my goal is to be able to dump the bitfields correctly, particularly if the structure is packed (for dumping a GDT for example).
> I just didn't manage to do it properly for the moment.
Okay, this should probably be a FIXME comment if you don't intend to handle it in the initial patch. It should also have a test case with some comments about the test behavior.


================
Comment at: lib/CodeGen/CGBuiltin.cpp:1206
+    QualType Arg0Type = Arg0->getType()->getPointeeType();
+    const RecordType *RT = Arg0Type->getAs<RecordType>();
+
----------------
You can use `const auto *` here because the type is spelled out in the initializer.


================
Comment at: lib/CodeGen/CGBuiltin.cpp:1211
+    RecordDecl *RD = RT->getDecl()->getDefinition();
+    ASTContext &ASTContext = RD->getASTContext();
+    const ASTRecordLayout &RL = ASTContext.getASTRecordLayout(RD);
----------------
Please don't name the variable the same name as a type.


================
Comment at: lib/CodeGen/CGBuiltin.cpp:1231
+      Types[getContext().VoidPtrTy] = "%p";
+      Types[getContext().FloatTy] = "%f";
+      Types[getContext().DoubleTy] = "%f";
----------------
It's unfortunate that you cannot distinguish between `float` and `double`. Do you need to use the format specifiers exactly?

What about other type information, like qualifiers or array extents? How should this handle types that do not exist in this mapping, like structure or enum types?


================
Comment at: lib/CodeGen/CGBuiltin.cpp:1238
+    /* field : RecordDecl::field_iterator */
+    for (auto *FD : RD->fields()) {
+      uint64_t Off = RL.getFieldOffset(FD->getFieldIndex());
----------------
`const auto *`


Repository:
  rC Clang

https://reviews.llvm.org/D44093