[PATCH] D44093: [BUILTINS] structure pretty printer

[Aaron Ballman via Phabricator via cfe-commits]

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