[PATCH] D44093: [BUILTINS] structure pretty printer

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: lib/Sema/SemaChecking.cpp:1122
+        !PtrArgType->getPointeeType()->isRecordType()) {
+      this->Diag(PtrArg->getLocStart(),
+                 diag::err_typecheck_convert_incompatible)
----------------
Drop all instances of `this->`.


================
Comment at: lib/Sema/SemaChecking.cpp:1156
+      if (!FT->isVariadic() || FT->getReturnType() != Context.IntTy ||
+          FT->getParamType(0) != firstArg) {
+        this->Diag(FnPtrArg->getLocStart(),
----------------
Rather than building up a type only for comparison purposes, why not check:
```
QualType PT = FT->getParamType(0);
if (!PT->isPointerType() || !PT->getPointeeType()->isCharType() || !PT->getPointeeType().isConstQualified()) {
  Diag(...);
}
```


================
Comment at: test/CodeGen/dump-struct-builtin.c:254
+  __builtin_dump_struct(&b, &printf);
+}
----------------
I'd like to see some test cases using unions (perhaps mixing unions and inner struct types), tests using arrays, floating-point values, and type qualifiers on the data members.


================
Comment at: test/Sema/builtin-dump-struct.c:8
+  void *b;
+  int (*goodfunc)(const char *, ...);
+  int (*badfunc1)(const char *);
----------------
Can you also add a test for: `int (*badfunc4)(char *, ...);` and `int (*badfunc5)();`


================
Comment at: test/Sema/builtin-dump-struct.c:15
+  __builtin_dump_struct(1);            // expected-error {{too few arguments to function call, expected 2, have 1}}
+  __builtin_dump_struct(1, 2);         // expected-error {{passing 'int' to parameter of incompatible type 'structure pointer type': type mismatch at 1st parameter ('int' vs 'structure pointer type')}}
+  __builtin_dump_struct(&a, 2);        // expected-error {{passing 'int' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int' vs 'int (*)(const char *, ...)')}}
----------------
Hrm, the `'structure pointer type'` in this diagnostic is unfortunate because it's being quoted as though it were a real type -- you could drop the single quotes. If you think the resulting diagnostic reads too strangely, perhaps we will have to go back to a custom diagnostic after all.


================
Comment at: test/Sema/builtin-dump-struct.c:17
+  __builtin_dump_struct(&a, 2);        // expected-error {{passing 'int' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int' vs 'int (*)(const char *, ...)')}}
+  __builtin_dump_struct(b, &goodfunc); // expected-error {{passing 'void *' to parameter of incompatible type 'structure pointer type': type mismatch at 1st parameter ('void *' vs 'structure pointer type')}}
+  __builtin_dump_struct(&a, badfunc1); // expected-error {{passing 'int (*)(const char *)' to parameter of incompatible type 'int (*)(const char *, ...)': type mismatch at 2nd parameter ('int (*)(const char *)' vs 'int (*)(const char *, ...)')}}
----------------
Why `&goodfunc`?


Repository:
  rC Clang

https://reviews.llvm.org/D44093