Re: patch via mailing list: Use getLocation() in too few/many arguments diagnostic
On Fri, Feb 21, 2020 at 10:54 AM John Marshall wrote: > > On 18 Feb 2020, at 16:40, Aaron Ballman wrote: > > Yup, I saw the failure on IRC and pushed up a change > > Thanks again Aaron. Now that the dust has settled, bug 23564 can be closed > (fixed by commit 260b91f379c) if someone with a bugzilla account wants to do > that. I didn't search further to see if there were other duplicate bug > reports. > > https://bugs.llvm.org/show_bug.cgi?id=23564 Thank you for pointing this out, I've closed the bug as well. ~Aaron > > John ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: patch via mailing list: Use getLocation() in too few/many arguments diagnostic
On 18 Feb 2020, at 16:40, Aaron Ballman wrote: > Yup, I saw the failure on IRC and pushed up a change Thanks again Aaron. Now that the dust has settled, bug 23564 can be closed (fixed by commit 260b91f379c) if someone with a bugzilla account wants to do that. I didn't search further to see if there were other duplicate bug reports. https://bugs.llvm.org/show_bug.cgi?id=23564 John ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: patch via mailing list: Use getLocation() in too few/many arguments diagnostic
On Tue, Feb 18, 2020 at 11:37 AM John Marshall
wrote:
>
> On 18 Feb 2020, at 16:24, Aaron Ballman wrote:
> >
> > I've commit on your behalf in
> > 260b91f379c8f86d3d6008648b3f2a945a007888, thank you for the patch!
>
> Thanks very much, Aaron.
>
> I regret to report that this appears to have broken an additional test [1]
> that I didn't see when running the test suite locally with "make
> check-clang". Patch (untested) attached.
Yup, I saw the failure on IRC and pushed up a change in
66c01627719f088bff9a0d09655a8ca0842b2c82 that should resolve it -- we
fixed the issue the same way.
~Aaron
>
> John
>
> [1]
> http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/23835/steps/test-check-all/logs/stdio
>
> --- a/clang/bindings/python/tests/cindex/test_diagnostics.py
> +++ b/clang/bindings/python/tests/cindex/test_diagnostics.py
> @@ -100,7 +100,7 @@ class TestDiagnostics(unittest.TestCase):
> self.assertRegexpMatches(children[0].spelling,
> '.*declared here')
> self.assertEqual(children[0].location.line, 1)
> -self.assertEqual(children[0].location.column, 1)
> +self.assertEqual(children[0].location.column, 6)
>
> def test_diagnostic_string_repr(self):
> tu = get_tu('struct MissingSemicolon{}')
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: patch via mailing list: Use getLocation() in too few/many arguments diagnostic
On 18 Feb 2020, at 16:24, Aaron Ballman wrote:
>
> I've commit on your behalf in
> 260b91f379c8f86d3d6008648b3f2a945a007888, thank you for the patch!
Thanks very much, Aaron.
I regret to report that this appears to have broken an additional test [1] that
I didn't see when running the test suite locally with "make check-clang". Patch
(untested) attached.
John
[1]
http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/23835/steps/test-check-all/logs/stdio
--- a/clang/bindings/python/tests/cindex/test_diagnostics.py
+++ b/clang/bindings/python/tests/cindex/test_diagnostics.py
@@ -100,7 +100,7 @@ class TestDiagnostics(unittest.TestCase):
self.assertRegexpMatches(children[0].spelling,
'.*declared here')
self.assertEqual(children[0].location.line, 1)
-self.assertEqual(children[0].location.column, 1)
+self.assertEqual(children[0].location.column, 6)
def test_diagnostic_string_repr(self):
tu = get_tu('struct MissingSemicolon{}')
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: patch via mailing list: Use getLocation() in too few/many arguments diagnostic
I've commit on your behalf in
260b91f379c8f86d3d6008648b3f2a945a007888, thank you for the patch!
~Aaron
On Mon, Feb 10, 2020 at 12:42 PM Aaron Ballman wrote:
>
> On Mon, Feb 10, 2020 at 10:06 AM John Marshall
> 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
> > > 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
> > AuthorDate: Mon Jan 20 14:58:14 2020 +
> > Commit: John Marshall
> > CommitDate: Mon Feb 10 14:30:58 2020 +
> >
> > 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
Re: patch via mailing list: Use getLocation() in too few/many arguments diagnostic
On Mon, Feb 10, 2020 at 10:06 AM John Marshall
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
> > 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
> AuthorDate: Mon Jan 20 14:58:14 2020 +
> Commit: John Marshall
> CommitDate: Mon Feb 10 14:30:58 2020 +
>
> 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
Re: patch via mailing list: Use getLocation() in too few/many arguments diagnostic
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.
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
> 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
AuthorDate: Mon Jan 20 14:58:14 2020 +
Commit: John Marshall
CommitDate: Mon Feb 10 14:30:58 2020 +
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__((v
Re: patch via mailing list: Use getLocation() in too few/many arguments diagnostic
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
wrote:
>
> I think this looks okay. I think Richard or Aaron might be able to provide a
> more informed opinion.
>
> -- HT
>
> On Fri, Feb 7, 2020 at 10:06 AM John Marshall via cfe-commits
> wrote:
>>
>> Ping. I am a newcomer to Clang so don't know who might be appropriate
>> reviewers to CC, so I've CCed a couple of general people from
>> clang/CODE_OWNERS.TXT who may be able to forward as appropriate.
>>
>> Thanks,
>>
>> John
>>
>>
>> On 20 Jan 2020, at 16:09, John Marshall wrote:
>> >
>> > This small patch improves the diagnostics when calling a function with the
>> > wrong number of arguments. For example, given
>> >
>> > int
>> > foo(int a, int b);
>> >
>> > int bar() { return foo(1); }
>> >
>> > The existing compiler shows the error message and a note for "foo declared
>> > here" showing only the "int" line. Ideally it would show the line
>> > containing the word "foo" instead, which it does after this patch. Also if
>> > the function declaration starts with some incidental macro, a trace of
>> > macro expansion is shown which is likely irrelevant. See for example
>> > https://github.com/samtools/htslib/issues/1013 and PR#23564.
>> >
>> > I have not contributed to LLVM before and I am unfamiliar with the
>> > difference between getBeginLoc() and getLocation() and the implications of
>> > using each. However this patch does fix PR#23564 and perhaps this fix
>> > would be mostly a no-brainer for those who are familiar with the code and
>> > these two location functions in particular?
>>
>> commit e8e73a04dd4274441541cc5fdc553cc4b26c00c3
>> Author: John Marshall
>> Date: Mon Jan 20 14:58:14 2020 +
>>
>> Use getLocation() in too few/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.
>>
>> 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);
>>
>> ___
>> cfe-commits mailing list
>> cfe-commits@lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: patch via mailing list: Use getLocation() in too few/many arguments diagnostic
I think this looks okay. I think Richard or Aaron might be able to provide
a more informed opinion.
-- HT
On Fri, Feb 7, 2020 at 10:06 AM John Marshall via cfe-commits <
cfe-commits@lists.llvm.org> wrote:
> Ping. I am a newcomer to Clang so don't know who might be appropriate
> reviewers to CC, so I've CCed a couple of general people from
> clang/CODE_OWNERS.TXT who may be able to forward as appropriate.
>
> Thanks,
>
> John
>
>
> On 20 Jan 2020, at 16:09, John Marshall wrote:
> >
> > This small patch improves the diagnostics when calling a function with
> the wrong number of arguments. For example, given
> >
> > int
> > foo(int a, int b);
> >
> > int bar() { return foo(1); }
> >
> > The existing compiler shows the error message and a note for "foo
> declared here" showing only the "int" line. Ideally it would show the line
> containing the word "foo" instead, which it does after this patch. Also if
> the function declaration starts with some incidental macro, a trace of
> macro expansion is shown which is likely irrelevant. See for example
> https://github.com/samtools/htslib/issues/1013 and PR#23564.
> >
> > I have not contributed to LLVM before and I am unfamiliar with the
> difference between getBeginLoc() and getLocation() and the implications of
> using each. However this patch does fix PR#23564 and perhaps this fix would
> be mostly a no-brainer for those who are familiar with the code and these
> two location functions in particular?
>
> commit e8e73a04dd4274441541cc5fdc553cc4b26c00c3
> Author: John Marshall
> Date: Mon Jan 20 14:58:14 2020 +
>
> Use getLocation() in too few/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.
>
> 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);
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: patch via mailing list: Use getLocation() in too few/many arguments diagnostic
Ping. I am a newcomer to Clang so don't know who might be appropriate reviewers
to CC, so I've CCed a couple of general people from clang/CODE_OWNERS.TXT who
may be able to forward as appropriate.
Thanks,
John
On 20 Jan 2020, at 16:09, John Marshall wrote:
>
> This small patch improves the diagnostics when calling a function with the
> wrong number of arguments. For example, given
>
> int
> foo(int a, int b);
>
> int bar() { return foo(1); }
>
> The existing compiler shows the error message and a note for "foo declared
> here" showing only the "int" line. Ideally it would show the line containing
> the word "foo" instead, which it does after this patch. Also if the function
> declaration starts with some incidental macro, a trace of macro expansion is
> shown which is likely irrelevant. See for example
> https://github.com/samtools/htslib/issues/1013 and PR#23564.
>
> I have not contributed to LLVM before and I am unfamiliar with the difference
> between getBeginLoc() and getLocation() and the implications of using each.
> However this patch does fix PR#23564 and perhaps this fix would be mostly a
> no-brainer for those who are familiar with the code and these two location
> functions in particular?
commit e8e73a04dd4274441541cc5fdc553cc4b26c00c3
Author: John Marshall
Date: Mon Jan 20 14:58:14 2020 +
Use getLocation() in too few/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.
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);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: patch via mailing list: Use getLocation() in too few/many arguments diagnostic
This patch has been languishing for 10 days, and it has been pointed out on
cfe-dev that it is important to directly CC appropriate reviewers. As there's
no-one listed in clang/CODE_OWNERS.TXT for diagnostics, I've now CCed "all
other parts" Richard.
On 20 Jan 2020, at 16:09, John Marshall via cfe-commits
wrote:
>
> This small patch improves the diagnostics when calling a function with the
> wrong number of arguments. For example, given
>
> int
> foo(int a, int b);
>
> int bar() { return foo(1); }
>
> The existing compiler shows the error message and a note for "foo declared
> here" showing only the "int" line. Ideally it would show the line containing
> the word "foo" instead, which it does after this patch. Also if the function
> declaration starts with some incidental macro, a trace of macro expansion is
> shown which is likely irrelevant. See for example
> https://github.com/samtools/htslib/issues/1013 and PR#23564.
>
> I have not contributed to LLVM before and I am unfamiliar with the difference
> between getBeginLoc() and getLocation() and the implications of using each.
> However this patch does fix PR#23564 and perhaps this fix would be mostly a
> no-brainer for those who are familiar with the code and these two location
> functions in particular?
>
> Thanks,
>
>John
commit 0fcec5ffe9bc048907f623635b3acf8731ac9ffd
Author: John Marshall
Date: Mon Jan 20 14:58:14 2020 +
Use getLocation() in too few/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.
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 2380a6b8d67..20c33c40c64 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -5193,7 +5193,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;
}
@@ -5238,7 +5238,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);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
patch via mailing list: Use getLocation() in too few/many arguments diagnostic
This small patch improves the diagnostics when calling a function with the
wrong number of arguments. For example, given
int
foo(int a, int b);
int bar() { return foo(1); }
The existing compiler shows the error message and a note for "foo declared
here" showing only the "int" line. Ideally it would show the line containing
the word "foo" instead, which it does after this patch. Also if the function
declaration starts with some incidental macro, a trace of macro expansion is
shown which is likely irrelevant. See for example
https://github.com/samtools/htslib/issues/1013 and PR#23564.
I have not contributed to LLVM before and I am unfamiliar with the difference
between getBeginLoc() and getLocation() and the implications of using each.
However this patch does fix PR#23564 and perhaps this fix would be mostly a
no-brainer for those who are familiar with the code and these two location
functions in particular?
Thanks,
John
commit 12c8979abaf40aa76c6769d6270f3565d71f3011
Author: John Marshall
Date: Mon Jan 20 14:58:14 2020 +
Use getLocation() in too few/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.
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index ea4b93ee6a5..19dfc7c7fd7 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);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
