Any updates on if my latest patch incorporating Jordy's suggestions is acceptable and ready for commit by someone? Attached again is the patch for reference.
Thanks, Terry
PR11857v3.patch
Description: Binary data
Am 11.05.2012 um 15:49 schrieb Terry Long:
> Here's another patch which implements all of the suggested changes.
>
> -Terry
>
> <PR11857v3.patch>
> 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 <[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
>
> _______________________________________________
> 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
