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
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
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
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
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
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
