patch via mailing list: Use getLocation() in too few/many arguments diagnostic

[Aaron Ballman via cfe-commits]

On Mon, Feb 10, 2020 at 10:06 AM John Marshall
<John.W.Marshall at glasgow.ac.uk> wrote:
>
> Thanks Aaron (and Hubert).
>
> I've attached an updated patch that now includes new test cases alongside some existing "too few / too many" test cases in test/Sema/exprs.c. This splits the function declaration over two lines so it can use -verify to validate the source location's line (but not column). If you'd prefer a FileCheck approach to get the column too, I'm happy to do that but please advise whether it would be best to create a new test/Sema/foo.c file for these tests or to add to one of the existing test files.
>
> Verified that without the patch, the notes are on the "MY_EXPORT void" line and the test cases fail. All tests still pass after this, after adjusting one existing FileCheck-based test case that also happens to exercise the patch's change.

Thank you for the patch -- I think it is fine to not check the column
for the cases you've added, so this iteration LGTM. I'm happy to
commit this on your behalf, but it will have to wait until next week
because I'm out at wg21 meetings this week. If no one else commits
this before I get home, I'll handle it then.

~Aaron

>
>     John
>
>
> On 7 Feb 2020, at 15:40, Aaron Ballman wrote:
> > Thank you for the patch -- I think the changes look reasonable, but it
> > should come with some test cases as well. Source location stuff is a
> > bit onerous to try to test, but I think the best approach would be to
> > add a new test that uses FileCheck instead of -verify so that you can
> > validate the source location's line and column are as expected in the
> > note. Once you have such a test (and have verified that no other tests
> > fail with your changes), I'm happy to commit on your behalf.
> >
> > ~Aaron
> >
> > On Fri, Feb 7, 2020 at 10:23 AM Hubert Tong
> > <hubert.reinterpretcast at gmail.com> wrote:
> >>
> >> I think this looks okay. I think Richard or Aaron might be able to provide a more informed opinion.
> >>
> >> -- HT
>
>
> commit cbd4a4a155b40dc77c2ed82f397fe303dfc10837
> Author:     John Marshall <John.W.Marshall at glasgow.ac.uk>
> AuthorDate: Mon Jan 20 14:58:14 2020 +0000
> Commit:     John Marshall <John.W.Marshall at glasgow.ac.uk>
> CommitDate: Mon Feb 10 14:30:58 2020 +0000
>
>     Use getLocation() in "too few/too many arguments" diagnostic
>
>     Use the more accurate location when emitting the location of the
>     function being called's prototype in diagnostics emitted when calling
>     a function with an incorrect number of arguments.
>
>     In particular, avoids showing a trace of irrelevant macro expansions
>     for "MY_EXPORT static int AwesomeFunction(int, int);". Fixes PR#23564.
>
>     Add test cases alongside other "too few/too many arguments" tests.
>     Adjust column position in incidentally related FileCheck-based test.
>
> diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
> index ffe72c98356..b9d7024f083 100644
> --- a/clang/lib/Sema/SemaExpr.cpp
> +++ b/clang/lib/Sema/SemaExpr.cpp
> @@ -5194,7 +5194,7 @@ Sema::ConvertArgumentsForCall(CallExpr *Call, Expr *Fn,
>
>        // Emit the location of the prototype.
>        if (!TC && FDecl && !FDecl->getBuiltinID() && !IsExecConfig)
> -        Diag(FDecl->getBeginLoc(), diag::note_callee_decl) << FDecl;
> +        Diag(FDecl->getLocation(), diag::note_callee_decl) << FDecl;
>
>        return true;
>      }
> @@ -5239,7 +5239,7 @@ Sema::ConvertArgumentsForCall(CallExpr *Call, Expr *Fn,
>
>        // Emit the location of the prototype.
>        if (!TC && FDecl && !FDecl->getBuiltinID() && !IsExecConfig)
> -        Diag(FDecl->getBeginLoc(), diag::note_callee_decl) << FDecl;
> +        Diag(FDecl->getLocation(), diag::note_callee_decl) << FDecl;
>
>        // This deletes the extra arguments.
>        Call->shrinkNumArgs(NumParams);
> diff --git a/clang/test/Misc/serialized-diags.c b/clang/test/Misc/serialized-diags.c
> index e401477a2eb..2f4b86fb42f 100644
> --- a/clang/test/Misc/serialized-diags.c
> +++ b/clang/test/Misc/serialized-diags.c
> @@ -56,7 +56,7 @@ void rdar11040133() {
>  // CHECK: Range: {{.*[/\\]}}serialized-diags.c:22:13 {{.*[/\\]}}serialized-diags.c:22:18
>  // CHECK: +-{{.*[/\\]}}serialized-diags.c:20:15: note: expanded from macro 'false' []
>  // CHECK: +-Range: {{.*[/\\]}}serialized-diags.c:20:15 {{.*[/\\]}}serialized-diags.c:20:16
> -// CHECK: +-{{.*[/\\]}}serialized-diags.c:19:1: note: 'taz' declared here []
> +// CHECK: +-{{.*[/\\]}}serialized-diags.c:19:6: note: 'taz' declared here []
>  // CHECK: {{.*[/\\]}}serialized-diags.h:5:7: warning: incompatible integer to pointer conversion initializing 'char *' with an expression of type 'int' [-Wint-conversion]
>  // CHECK: Range: {{.*[/\\]}}serialized-diags.h:5:16 {{.*[/\\]}}serialized-diags.h:5:17
>  // CHECK: +-{{.*[/\\]}}serialized-diags.c:26:10: note: in file included from {{.*[/\\]}}serialized-diags.c:26: []
> diff --git a/clang/test/Sema/exprs.c b/clang/test/Sema/exprs.c
> index 760c45e02f3..4e144041aca 100644
> --- a/clang/test/Sema/exprs.c
> +++ b/clang/test/Sema/exprs.c
> @@ -163,12 +163,15 @@ void test17(int x) {
>    x = sizeof(x/0);  // no warning.
>  }
>
> -// PR6501 & PR11857
> +// PR6501, PR11857, and PR23564
>  void test18_a(int a); // expected-note 2 {{'test18_a' declared here}}
>  void test18_b(int); // expected-note {{'test18_b' declared here}}
>  void test18_c(int a, int b); // expected-note 2 {{'test18_c' declared here}}
>  void test18_d(int a, ...); // expected-note {{'test18_d' declared here}}
>  void test18_e(int a, int b, ...); // expected-note {{'test18_e' declared here}}
> +#define MY_EXPORT __attribute__((visibility("default")))
> +MY_EXPORT void // (no "declared here" notes on this line, no "expanded from MY_EXPORT" notes either)
> +test18_f(int a, int b); // expected-note 2 {{'test18_f' declared here}}
>  void test18(int b) {
>    test18_a(b, b); // expected-error {{too many arguments to function call, expected single argument 'a', have 2}}
>    test18_a(); // expected-error {{too few arguments to function call, single argument 'a' was not specified}}
> @@ -177,6 +180,8 @@ void test18(int b) {
>    test18_c(b, b, b); // expected-error {{too many arguments to function call, expected 2, have 3}}
>    test18_d(); // expected-error {{too few arguments to function call, at least argument 'a' must be specified}}
>    test18_e(); // expected-error {{too few arguments to function call, expected at least 2, have 0}}
> +  test18_f(b); // expected-error {{too few arguments to function call, expected 2, have 1}}
> +  test18_f(b, b, b); // expected-error {{too many arguments to function call, expected 2, have 3}}
>  }
>
>  typedef int __attribute__((address_space(256))) int_AS256;
>