> 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 <[email protected]> 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 <[email protected]> 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
>> [email protected]
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> 

_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Reply via email to