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

[Richard Smith]

Thanks, r156807!

On Mon, May 14, 2012 at 11:14 PM, Terry Long <macmantrl at me.com> wrote:

> OK, made that update. Here is the new patch.
>
>
> -Terry
>
>
> Am 15.05.2012 um 01:45 schrieb Richard Smith:
>
> Can you add the word 'arguments' to the end of these diagnostics:
>
> +  test18_a(b, b); // expected-error {{too many arguments to function
> call, expected single argument 'a', have 2}}
> +    j(2, 3); // expected-error{{too many arguments to function call,
> expected at most single argument 'f', have 2}}
>
> Other than that, I think this is fine.
>
> On Mon, May 14, 2012 at 10:36 PM, Terry Long <macmantrl at me.com> wrote:
>
>> 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
>>
>>
>>
>>
>> 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 <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
>> >
>> > _______________________________________________
>> > cfe-commits mailing list
>> > cfe-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>>
>>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120514/c9495b73/attachment.html>