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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
