[C++0x] Range-based for statements and ADL

Hi again.

Here it is my first try at this. I have changed the list to
gcc-patches, I don't know if cross post would be correct.
Please, note that this patch is not finished: the new test cases are
still missing, and expect format mistakes, misspellings and the
like...

A few comments:

1. I'm not sure about what should happen if the begin/end found in
class scope are not ordinary functions. My guess is that if it is a
function (static or non-static) it is called normally, and if it is a
member variable it is searched for an operator()(). If it is a type it
should fail. That is what I tried to implement (I think it works).

2. The function cp_parser_perform_range_for_lookup() does the same job
twice, but interleaved, so it's not easy to avoid typing everything
once and again. Maybe it should be split in several pieces.
Suggestions welcome.

3. In the function cp_parser_perform_range_for_lookup() I made quite a
shameful, but clever, abuse of the local variables and arguments. I've
seen similar patterns in the code, but if you think it is too much, I
can rewrite it.

4. The approach to error diagnostics can be done in so many ways that
I cannot make my mind. I think that the attached patch does a
reasonable job, but again...

5. Hidden in the new wording, there are a restriction about arrays of
incomplete type or unknown size. That restriction should have been
obvious before (easy to say now ;-), and it is quite unrelated to the
other changes. But I don't know if it is worth its own patch (3 lines
an a future test).

Awaiting for comments.

--
Rodrigo

On 29 March 2011 01:49, Rodrigo Rivas wrote:
> Hi again.
>
> Here it is my first try at this. I have changed the list to
> gcc-patches, I don't know if cross post would be correct.
> Please, note that this patch is not finished: the new test cases are
> still missing, and expect format mistakes, misspellings and the
> like...
>
> A few comments:
>
> 1. I'm not sure about what should happen if the begin/end found in
> class scope are not ordinary functions. My guess is that if it is a
> function (static or non-static) it is called normally, and if it is a
> member variable it is searched for an operator()(). If it is a type it
> should fail. That is what I tried to implement (I think it works).

The wording was carefully written so that if name lookup finds any
"r.begin" then it will try "r.begin()" so yes, it should be able to
call data members of callable type.

> 2. The function cp_parser_perform_range_for_lookup() does the same job
> twice, but interleaved, so it's not easy to avoid typing everything
> once and again. Maybe it should be split in several pieces.
> Suggestions welcome.
>
> 3. In the function cp_parser_perform_range_for_lookup() I made quite a
> shameful, but clever, abuse of the local variables and arguments. I've
> seen similar patterns in the code, but if you think it is too much, I
> can rewrite it.
>
> 4. The approach to error diagnostics can be done in so many ways that
> I cannot make my mind. I think that the attached patch does a
> reasonable job, but again...

Should we consistently refer to %<for%> so the keyword is highlighted?

> 5. Hidden in the new wording, there are a restriction about arrays of
> incomplete type or unknown size. That restriction should have been
> obvious before (easy to say now ;-), and it is quite unrelated to the
> other changes. But I don't know if it is worth its own patch (3 lines
> an a future test).
>
> Awaiting for comments.
>
> --
> Rodrigo
>

On Tue, Mar 29, 2011 at 10:12 AM, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> Should we consistently refer to %<for%> so the keyword is highlighted?
Now that you say... I've not been quite consistent. We could say
"range-based %<for%>", with only a dash between 'range' and 'based'.

On Tue, Mar 29, 2011 at 4:38 AM, Rodrigo Rivas
<rodrigorivascosta@gmail.com> wrote:
> On Tue, Mar 29, 2011 at 10:12 AM, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
>> Should we consistently refer to %<for%> so the keyword is highlighted?
> Now that you say... I've not been quite consistent. We could say
> "range-based %<for%>", with only a dash between 'range' and 'based'.
>

or "new-style for loop"?

On Tue, Mar 29, 2011 at 5:46 PM, Gabriel Dos Reis
<gdr@integrable-solutions.net> wrote:
> or "new-style for loop"?
Well, that is what they are called in Java, isn't it? And the syntax
is just the same, so it would make kind of sense.
But in the C++0x draft and the GCC docs it is almost always called
"range-based for loops, so I think it is preferred.


Talking of this... I tried the simplest wrong range-based for, with my
latest patch:

int main()
{
for (int x : 0)
    ;
}

And that is what I got:

test.cpp: In function ‘int main()’:
test.cpp:3:18: error: ‘begin’ was not declared in this scope
test.cpp:3:18: error: ‘end’ was not declared in this scope

That's ok. But now, if I add #include <vector> I get:

test.cpp: In function ‘int main()’:
test.cpp:4:18: error: no matching function for call to ‘begin(int&)’
test.cpp:4:18: note: candidates are:
/home/rodrigo/local/gcc/lib/gcc/i686-pc-linux-gnu/4.7.0/../../../../include/c++/4.7.0/bits/range_access.h:87:5:
note: template<class _Tp, unsigned int _Nm> _Tp* std::begin(_Tp
(&)[_Nm])
/home/rodrigo/local/gcc/lib/gcc/i686-pc-linux-gnu/4.7.0/../../../../include/c++/4.7.0/bits/range_access.h:58:63:
note: template<class _Container> decltype (__cont->begin())
std::begin(const _Container&)
/home/rodrigo/local/gcc/lib/gcc/i686-pc-linux-gnu/4.7.0/../../../../include/c++/4.7.0/bits/range_access.h:48:57:
note: template<class _Container> decltype (__cont->begin())
std::begin(_Container&)
/home/rodrigo/local/gcc/lib/gcc/i686-pc-linux-gnu/4.7.0/../../../../include/c++/4.7.0/initializer_list:86:38:
note: template<class _Tp> constexpr const _Tp*
std::begin(std::initializer_list<_Tp>)
test.cpp:4:18: error: no matching function for call to ‘end(int&)’
test.cpp:4:18: note: candidates are:
/home/rodrigo/local/gcc/lib/gcc/i686-pc-linux-gnu/4.7.0/../../../../include/c++/4.7.0/bits/range_access.h:97:5:
note: template<class _Tp, unsigned int _Nm> _Tp* std::end(_Tp
(&)[_Nm])
/home/rodrigo/local/gcc/lib/gcc/i686-pc-linux-gnu/4.7.0/../../../../include/c++/4.7.0/bits/range_access.h:78:59:
note: template<class _Container> decltype (__cont->end())
std::end(const _Container&)
/home/rodrigo/local/gcc/lib/gcc/i686-pc-linux-gnu/4.7.0/../../../../include/c++/4.7.0/bits/range_access.h:68:53:
note: template<class _Container> decltype (__cont->end())
std::end(_Container&)
/home/rodrigo/local/gcc/lib/gcc/i686-pc-linux-gnu/4.7.0/../../../../include/c++/4.7.0/initializer_list:96:36:
note: template<class _Tp> constexpr const _Tp*
std::end(std::initializer_list<_Tp>)

Although the error messages are technically correct, I find them too
verbose and probably misleading to the novice: "'begin' not declared?
I'm not using 'begin', I'm using 'for').
Note also that the error could be corrected adding member functions
begin/end (if the range were of class type), but that is not suggested
in the error message.

Maybe a simpler error is better. such as:
"expression is not valid as range in a range-based %<for%> loop
because it lacks begin/end members or begin(int&) and end(int&)
overloads." (I leave the wording to someone more skilled with
English).

Regards.
--
Rodrigo

On 29 March 2011 17:41, Rodrigo Rivas wrote:
>
> Maybe a simpler error is better. such as:
> "expression is not valid as range in a range-based %<for%> loop
> because it lacks begin/end members or begin(int&) and end(int&)
> overloads." (I leave the wording to someone more skilled with
> English).

If you can prevent the full lookup error and print something different
I think users would be very grateful indeed.

How about "No suitable %<begin%> and %<end%> functions found for range
expression of type %qT in %<for%> statement" ?

That

On Tue, Mar 29, 2011 at 8:22 PM, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> How about "No suitable %<begin%> and %<end%> functions found for range
> expression of type %qT in %<for%> statement" ?

Nice.
But the problem here is that there are a lot of different error conditions:
1. begin member but not end member.
2. end member but not begin member.
3. begin and end member but one or both are not callable/accesible/whatever.
3. no begin/end members, a begin global function but not end.
4. no begin/end members, an end global function but not begin.
5. no begin/end members, begin and end functions but one or both not
callable/wrong overload/etc.
6. no begin/end members, no begin/end global functions. Since the
standard headers have the std::begin/std::end templates, this will not
happen when you include (most) STL headers.

What is the best message for each? Does it make sense to have up to 6
different messages? Too many?
I just can't tell.
--
Rodrigo

On 29 March 2011 21:33, Rodrigo Rivas wrote:
> On Tue, Mar 29, 2011 at 8:22 PM, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
>> How about "No suitable %<begin%> and %<end%> functions found for range
>> expression of type %qT in %<for%> statement" ?
>
> Nice.
> But the problem here is that there are a lot of different error conditions:
> 1. begin member but not end member.
> 2. end member but not begin member.

I think these cases should say something like "invalid lookup results,
member 'begin' and non-member 'end'" (or vice versa) as we had in last
week's patch.

Or maybe it would be better to say "lookup for 'for' found member
'begin' but no member 'end'"

> 3. begin and end member but one or both are not callable/accesible/whatever.

I would definitely want to get an "access denied", "deleted function"
etc. error for that case.

> 3. no begin/end members, a begin global function but not end.
> 4. no begin/end members, an end global function but not begin.

Maybe something similar to the text I suggested above, but only
mentioning the lookup that failed, "no suitable 'begin' found for
range expression ..."

> 5. no begin/end members, begin and end functions but one or both not
> callable/wrong overload/etc.

In some cases I would probably want to know the actual error, not just
"cannot use range-based for"

If a begin() is found but can't be called, showing the "candidates
are..." list might be helpful to see the problem e.g. if
my::begin(my::type&) can't be called for const my::type.

> 6. no begin/end members, no begin/end global functions. Since the
> standard headers have the std::begin/std::end templates, this will not
> happen when you include (most) STL headers.

Maybe this is the only case where the text I suggested is useful, I
think a special "can't use range-based for" makes sense in this case,
instead of "begin not declared in this scope"

> What is the best message for each? Does it make sense to have up to 6
> different messages? Too many?

It would be nice for users, but it's certainly more work, and
potentially more places for bugs to creep in.

On Tue, Mar 29, 2011 at 11:13 PM, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
Thank you for your suggestions!
IMO, error cases 3 (hey, two 3s!), 4 and 6 are not so likely, as
including any STL container header will make a begin and an end
functions declared, though maybe not usabe. In these cases the most
probable error case would be 5.

> It would be nice for users, but it's certainly more work, and
> potentially more places for bugs to creep in.
Yeah, you're right. I think I'll leave that for a future patch...

Rodrigo