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

[Aaron Ballman via cfe-commits]

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
>
> On Fri, Feb 7, 2020 at 10:06 AM John Marshall via cfe-commits <cfe-commits at 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.Marshall at 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 at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits