[PATCH] D44093: [BUILTINS] structure pretty printer

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Sorry for the delayed review; I'm back from vacation now and am picking things up again.



================
Comment at: test/Sema/builtin-dump-struct.c:8
+  void *b;
+  int (*goodfunc)(const char *, ...);
+  int (*badfunc1)(const char *);
----------------
paulsemel wrote:
> aaron.ballman wrote:
> > Can you also add a test for: `int (*badfunc4)(char *, ...);` and `int (*badfunc5)();`
> Isn't `int (*func)()` is a valid prototype for a printf like function in C ?
> I instead added `int (*func)(void)` to the test cases.
> Isn't int (*func)() is a valid prototype for a printf like function in C ?

No, because it's missing the `const char *` as the mandatory first parameter. Do you want that to be allowed and hope the callee has it correct on their side, or do you want it to diagnose as not being a valid function?


================
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 *, ...)')}}
----------------
paulsemel wrote:
> aaron.ballman wrote:
> > 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.
> I think it will be better to just put something like `structure pointer`, so that we understand the type we are talking about.
> But this diagnostic seems great, still, what do you think about sticking with it ?
I think it's fine now; if it turns out to be horribly confusing to users, we can address it later.


================
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 *, ...)')}}
----------------
paulsemel wrote:
> aaron.ballman wrote:
> > Why `&goodfunc`?
> Yes, we already have a test like this anyway :)
It was more a question of why the ampersand on the second argument -- I think that can be dropped and it'll be consistent with the rest of the tests.


Repository:
  rC Clang

https://reviews.llvm.org/D44093