[PATCH] D44093: [BUILTINS] structure pretty printer

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added reviewers: echristo, rsmith.
aaron.ballman added a comment.

Adding some reviewers.

One thing that's missing from this patch are test cases that demonstrate both the semantic checking (builtin accepts proper args, rejects improper ones, etc) and output.



================
Comment at: lib/CodeGen/CGBuiltin.cpp:1204-1205
+
+    const Expr *e = E->getArg(0)->IgnoreImpCasts();
+    QualType type = e->getType()->getPointeeType();
+    const RecordType *RT = type->getAs<RecordType>();
----------------
These declarations do not meet our coding style guidelines -- you should pick some new names (elsewhere as well).


================
Comment at: lib/CodeGen/CGBuiltin.cpp:1208
+
+    assert(RT && "The first argument must be a record type");
+
----------------
I don't see anything enforcing this constraint, so users are likely to hit this assertion rather than a compile error.


================
Comment at: lib/CodeGen/CGBuiltin.cpp:1211
+    RecordDecl *RD = RT->getDecl()->getDefinition();
+    auto &ASTContext = RD->getASTContext();
+    const ASTRecordLayout &RL = ASTContext.getASTRecordLayout(RD);
----------------
Please don't use `auto` when the type is not explicitly spelled out in the initializer.


================
Comment at: lib/CodeGen/CGBuiltin.cpp:1218-1232
+    Types[getContext().CharTy] = "%c";
+    Types[getContext().BoolTy] = "%d";
+    Types[getContext().IntTy] = "%d";
+    Types[getContext().UnsignedIntTy] = "%u";
+    Types[getContext().LongTy] = "%ld";
+    Types[getContext().UnsignedLongTy] = "%lu";
+    Types[getContext().LongLongTy] = "%lld";
----------------
These should only be set up one time rather than each time someone calls the builtin.


================
Comment at: lib/CodeGen/CGBuiltin.cpp:1235-1236
+    /* field : RecordDecl::field_iterator */
+    for (auto field = RD->field_begin(); field != RD->field_end(); field++)
+    {
+      uint64_t off = RL.getFieldOffset(field->getFieldIndex());
----------------
Can be replaced with a range-based for loop over `fields()`.

Also, the formatting of the braces doesn't match the coding standard (happens elsewhere as well) -- you should run your patch through clang-format: https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting


================
Comment at: lib/CodeGen/CGBuiltin.cpp:1258
+
+      //Need to handle bitfield here
+
----------------
Are you intending to implement this as part of this functionality?


Repository:
  rC Clang

https://reviews.llvm.org/D44093