[cfe-commits] [PATCH] "missing argument" diagnostic should include argument name

Terry Long macmantrl at me.com
Fri May 11 12:49:15 PDT 2012


Here's another patch which implements all of the suggested changes.

-Terry

-------------- next part --------------
A non-text attachment was scrubbed...
Name: PR11857v3.patch
Type: application/octet-stream
Size: 9596 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120511/fd229af2/attachment.obj>
-------------- next part --------------

Am 11.05.2012 um 13:16 schrieb Terry Long:

>> Sorry for weighing in so late, but some of the messages don't seem quite right to me. A "single" could help a lot for some of these cases (suggested fixes in brackets):
>> 
>>> too few arguments to function call, [single] argument 'a' was not specified
>> 
> 
> I agree, using 'single' helps clarify that the function takes only one argument.
> 
>> 
>> Without the "single" I feel like this is a warning for /any/ "too few args" situation that's only missing one arg (e.g. 2 for 3).
>> 
>>> candidate function not viable: requires [single] argument 'n', but 2 [arguments] were provided
>> 
>> This doesn't feel like valid English as written. Two whats? ("I was going to go to the state of Hawaii, but I went to two instead.") And here the "single" really underscores that the problem is too many arguments.
> 
> I originally had 'arguments' in the message, but took it out assuming "argument 'n'" was enough to know we were talking about arguments. Given your Hawaii example, I can see that the more correct way is probably to explicitly say 'arguments'.
> 
>> 
>>> candidate function not viable: requires[*] at most argument 'n', but 2 [arguments] were provided
>> 
>> 
>> * s/requires/allows? In this specific case of "0 or 1" it seems more fitting; not sure about the other "at most" warnings. Also "arguments", same as above.
> 
> Yes, allows is more fitting. A function with 1 optional argument is not requiring anything.
> 
>> 
>>> candidate function not viable: requires at least argument 'n', but none[*] were provided
>> 
>> s/none/no arguments/, same as above.
>> 
>> Also, why no version for err_typecheck_call_too_many_args, since the overload resolution gets one for too many args?
> 
> I overlooked this on my first attempt at the patch, and only added it to the 'candidate not viable' diagnostic because the same diagnostic was being used for /at most/at least/exactly/. I will definitely add this in.
> 
>> 
>>> too many arguments to function call, expected single argument 'n', have 2 [arguments]
>> 
>> Here I could go either way on including the last "arguments", since it was already stated at the beginning.
> 
> I would personally leave out 'arguments' to be consistent with all the other cases which use 'expected 2, have 0' form, and it is implied by the beginning phrase.
> 
>> 
>> What do you think?
> 
> I will create a new patch with these changes unless anyone else objects.
> 
> 
> -Terry
> 
> 
>> Jordy
>> 
>> 
>> On May 11, 2012, at 1:18, Richard Smith wrote:
>> 
>>> Great, thanks for working on this! Committed as r156607.
>>> 
>>> On Thu, May 10, 2012 at 7:31 PM, Terry Long <macmantrl at me.com> wrote:
>>> I've added more test coverage, removed deprecated methods, and extended the enhancement to the 'candidate function not viable' diagnostic for C++.
>>> 
>>> Patch version 2 attached.
>>> 
>>> -Terry Long
>>> 
>>> 
>>> 
>>> 
>>> Am 10.05.2012 um 19:17 schrieb Richard Smith:
>>> 
>>>> On Thu, May 10, 2012 at 4:00 PM, Terry Long <macmantrl at me.com> wrote:
>>>>> The patch generally looks good, thanks!
>>>> 
>>>> Great, thanks for the feedback.
>>>> 
>>>>> Presumably this only applies to the case where there are no arguments, because otherwise we couldn't know /which/ argument was missing?
>>>> 
>>>> Yes, only for the case where there are no arguments to a function that takes 1 argument. Almost impossible to determine the missing argument(s) otherwise.
>>>> 
>>>>> Please add test coverage for the err_typecheck_call_too_few_args_at_least_one diagnostic. Also, NamedDecl::getNameAsString is deprecated; please just use "<< FDecl->getParamDecl(0)", and use getParamDecl(0)->getDeclName()'s operator bool() in the test, rather than empty().
>>>> 
>>>> OK, I'll update this. I was using the online doxygen docs and didn't see any deprecation warnings. Anywhere where I can find that information?
>>>> 
>>>> It's in include/clang/AST/Decl.h:138-141, though for some reason those comments aren't exposed to doxygen...
>>>> 
>>>>> It would also be great to extend this to the 'candidate function not viable' diagnostics in C++.
>>>> 
>>>> I can take a look at this too.
>>>> 
>>>> Awesome, thanks. 
>>> 
>>> 
>>> 
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>> 
> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits



More information about the cfe-commits mailing list