[PATCH] D44093: [BUILTINS] structure pretty printer

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Mar 5 13:58:54 PST 2018

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?

  rC Clang


More information about the cfe-commits mailing list