[cfe-commits] [patch] Better diagnostics in for-range expressions.

[Sam Panzer]

Here is the long-awaited patch that addresses Matthieu's suggestions. This
version runs the same for-range building code on the dereferenced pointer
as it did on the failing original, picking (and only showing the error
messages from) the most successful of the two. It does not handle pointers
to arrays (or containers/smart pointers that overload operator*), though
this could be added later.

Existing test cases have been updated to match again.

On Fri, Jun 8, 2012 at 3:06 PM, Sam Panzer <panzer at google.com> wrote:

> Yep, the FixItHint is issued only when there are methods for both begin
> and end. I do not try to match a dereferenced pointer to non-method (ADL)
> functions, though I'm currently working on another change that does. The
> patch submitted previously would just issue the FixItHint if begin and end
> methods exist, without doing any further checking, e.g. on their types.
>
> I will also adjust the "missing begin/end" notes as Matthieu suggested.
>
> -Sam
>
>
> On Fri, Jun 8, 2012 at 12:36 PM, Matthieu Monrocq <
> matthieu.monrocq at gmail.com> wrote:
>
>>
>>
>> On Fri, Jun 8, 2012 at 8:18 PM, Sam Panzer <panzer at google.com> wrote:
>>
>>> Ah. Yes. It's actually attached now :)
>>>
>>>
>>> On Fri, Jun 8, 2012 at 11:16 AM, Matt Beaumont-Gay <matthewbg at google.com
>>> > wrote:
>>>
>>>> ENOPATCH
>>>>
>>>> On Fri, Jun 8, 2012 at 11:13 AM, Sam Panzer <panzer at google.com> wrote:
>>>> > Hi,
>>>> >
>>>> > Richard Smith suggested that I try to improve the diagnostic emitted
>>>> when
>>>> > Clang encounters certain kinds of invalid C+11 ranged-based for loops.
>>>> >
>>>> >> When a pointer to a container is used as the range in a range-based
>>>> for,
>>>> >> Clang's diagnostic is not awesome:
>>>> >>
>>>> >>> struct S { int *begin(); int *end(); };
>>>> >>> void f(S *p) {
>>>> >>>  for (auto i : p) {}
>>>> >>> }
>>>> >>> tmp.cpp:3:15: error: use of undeclared identifier 'begin'
>>>> >>>  for (auto i : p) {} }
>>>> >>>              ^
>>>> >>> tmp.cpp:3:15: note: range has type 'S *'
>>>> >>
>>>> >>
>>>> >> We should do better than that, and suggest inserting the missing '*'.
>>>> >
>>>> >
>>>> > This patch replaces the errors complaining about undeclared
>>>> identifiers with
>>>> > an error specific to ranged-for loops, along with an explanatory
>>>> note. I
>>>> > also updated the existing test cases to reflect the change. For
>>>> example, the
>>>> > above code now generates this:
>>>> >
>>>> >> test.cpp:3:15: error: invalid range expression of type 'S *'
>>>> >>  for (auto i : p) {}
>>>> >>              ^ ~
>>>> >> test.cpp:3:16: note: range expression is of type 'S *'; did you
>>>> >>       mean to dereference it with '*'?
>>>> >>  for (auto i : p) {}
>>>> >>                ^
>>>> >>                *
>>>> >> test.cpp:3:14: note: no viable 'end' function for range of type
>>>> >>       'S *'
>>>> >>  for (auto i : p) {}
>>>> >
>>>> >
>>>> > I hope that this is helpful, and comments are always welcome.
>>>> >
>>>> > -Sam
>>>>
>>>
>> Just a couple remarks.
>>
>> First, this sounds like a prime example for a Fix-It, at the very
>> condition that there *is* a  begin  and  end  functions/methods available
>> on the dereferenced  p. Do you check by the way ?
>>
>> Second, I am not sure about the "missing begin" and "missing end" notes.
>> I would advise having them only conditionally:
>> - if dereferencing would make it okay, then let's remove them, they are
>> probably bogus
>> - if dereferencing does not help (nothing for S& either), then we
>> probably need to keep them
>>
>> -- Matthieu.
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120613/decbd086/attachment.html>