aaron.ballman added inline comments.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:5022-5023
+def err_dump_struct_invalid_argument_type : Error<
+ "invalid argument of type %0; expected %1">;
+
----------------
Can you look to see if we have an existing diagnostic that can cover this
instead?
================
Comment at: lib/CodeGen/CGBuiltin.cpp:1231
+ Types[getContext().VoidPtrTy] = "%p";
+ Types[getContext().FloatTy] = "%f";
+ Types[getContext().DoubleTy] = "%f";
----------------
paulsemel wrote:
> aaron.ballman wrote:
> > 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?
> So, I've think about it. What I am going to do is that if I do not know the
> type of the field, I am just going to print it as a pointer. I know that it
> is not the best solution, but I think it's a okay-ish solution until I
> implement the other types.
> For the qualifiers, at it is printed the same way with and without those, I
> can just add the entries in the DenseMap.
> So, I've think about it. What I am going to do is that if I do not know the
> type of the field, I am just going to print it as a pointer. I know that it
> is not the best solution, but I think it's a okay-ish solution until I
> implement the other types.
Eek. That seems unfortunate. I'm thinking about very common use cases, like:
```
struct S {
int i, j;
float x, y;
};
struct T {
struct S s;
int k;
};
```
Printing out `s` as a pointer seems... not particularly useful.
================
Comment at: lib/CodeGen/CGBuiltin.cpp:1218
+ Types[getContext().getConstType(type)] = format; \
+ Types[getContext().getVolatileType(type)] = format; \
+ Types[getContext().getConstType(getContext().getVolatileType(type))] =
format;
----------------
This seems insufficient, as there are other qualifiers (restrict, ObjC GC
qualifiers, etc). I think a better way to do this is to call
`QualType::getUnqualifiedType()` on the type accessing the map.
================
Comment at: lib/CodeGen/CGBuiltin.cpp:1252
+ Types[getContext().getPointerType(getContext().CharTy)] = "%s";
+ GENERATE_TYPE_QUALIFIERS_NO_RESTRICT(getContext().CharTy, "%s")
+ }
----------------
What about other types that have format specifiers (ptrdiff_t, size_t,
intptr_t, char16_t, etc)?
================
Comment at: lib/CodeGen/CGBuiltin.cpp:1255
+
+ /* field : RecordDecl::field_iterator */
+ for (const auto *FD : RD->fields()) {
----------------
I don't think this comment adds a lot of value.
================
Comment at: lib/CodeGen/CGBuiltin.cpp:1269-1270
+
+ /* If the type is not handled yet, let's just print the data as a pointer
+ */
+ if (Types.find(FD->getType()) == Types.end())
----------------
We generally prefer `//` style comments unless there's good reason to use `/*
*/`.
================
Comment at: lib/Sema/SemaChecking.cpp:1114
+ case Builtin::BI__builtin_dump_struct: {
+ // We check for argument number
+ if (checkArgCount(*this, TheCall, 2))
----------------
Comments should be grammatically correct, including punctuation (here and
elsewhere).
================
Comment at: lib/Sema/SemaChecking.cpp:1118
+ // Ensure that the first argument is of type 'struct XX *'
+ const Expr *Arg0 = TheCall->getArg(0)->IgnoreImpCasts();
+ if (!Arg0->getType()->isPointerType()) {
----------------
You probably want to use `IgnoreParenImpCasts()`.
================
Comment at: lib/Sema/SemaChecking.cpp:1119
+ const Expr *Arg0 = TheCall->getArg(0)->IgnoreImpCasts();
+ if (!Arg0->getType()->isPointerType()) {
+ this->Diag(Arg0->getLocStart(),
diag::err_dump_struct_invalid_argument_type)
----------------
Rather than calling `is` followed by `get`, you should just call `get` once and
check the results here.
================
Comment at: lib/Sema/SemaChecking.cpp:1121
+ this->Diag(Arg0->getLocStart(),
diag::err_dump_struct_invalid_argument_type)
+ << Arg0->getType() << "structure pointer type";
+ return ExprError();
----------------
The string literals should be part of a `%select` in the diagnostic itself
rather than printed this way.
================
Comment at: lib/Sema/SemaChecking.cpp:1132-1133
+ // Ensure that the second argument is of type 'FunctionType'
+ const Expr *Arg1 = TheCall->getArg(1)->IgnoreImpCasts();
+ if (!Arg1->getType()->isPointerType()) {
+ this->Diag(Arg1->getLocStart(),
diag::err_dump_struct_invalid_argument_type)
----------------
Same suggestions here.
Also, `Arg1` and `Arg0` aren't particularly descriptive names. Can you pick
names based on the semantics of the arguments rather than their position?
================
Comment at: lib/Sema/SemaChecking.cpp:1135
+ this->Diag(Arg1->getLocStart(),
diag::err_dump_struct_invalid_argument_type)
+ << Arg1->getType() << "printf like function pointer type";
+ return ExprError();
----------------
What is a "printf like function pointer type"?
================
Comment at: lib/Sema/SemaChecking.cpp:1155
+ }
+
+
----------------
Spurious newline.
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
