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

[Sam Panzer]

I will now be including the word 'attach' in these emails as Gmail's
equivalent of -Wmissing-patch.

On Wed, Jun 13, 2012 at 8:12 PM, Sean Silva <silvas at purdue.edu> wrote:

> I think you forgot to attach the patch.
>
> On Wed, Jun 13, 2012 at 2:14 PM, Sam Panzer <panzer at google.com> wrote:
>
>> 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.
>>>>
>>>
>>>
>>
>> _______________________________________________
>> 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/20120614/5106549e/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: range-diagnostic.patch
Type: application/octet-stream
Size: 26781 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120614/5106549e/attachment.obj>