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

[Matthieu Monrocq]

On Thu, Jun 14, 2012 at 7:32 PM, Sam Panzer <panzer at google.com> wrote:

> 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
>>>
>>>
>>
> Thank you for working on this!

I would agree with you that support for "more" can always be added after it
has been proven to work.

Now let's hope someone reviews the actual patch :)

-- Matthieu
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120614/1c24a396/attachment.html>