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 

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 

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: 

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


cfe-commits mailing list

Reply via email to