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.reinterpretc...@gmail.com> 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
> <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 <john.w.marsh...@glasgow.ac.uk>
>> Date: Mon Jan 20 14:58:14 2020 +0000
>>
>> 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
