[cfe-commits] [PATCH] C++0x range-based for loop

[Douglas Gregor]

On Apr 13, 2011, at 4:04 PM, Richard Smith wrote:

> Hi,
> 
> Attached is an updated for-range patch. The main differences from the
> previous patch are noted below.
> 
> On Sun, March 27, 2011 11:57, Douglas Gregor wrote:
>> On Mar 21, 2011, at 12:56 AM, Richard Smith wrote:
>>> The attached patch adds support for C++0x's range-based for loops to
>>> clang, according to the current draft (N3242). Specifically:
>>> 
>>> A new Stmt subclass, CXXForRangeStmt, has been added. This class
>>> provides two views of the for-statement: a partially-desugared
>>> representation (just enough desugaring is done to be able to perform the
>>> semantic checks needed), and an implicit source-as-written
>>> representation (extracted from the partially-desugared representation).
>>> 
>>> In order to avoid tentative parsing, the parsing of the
>>> for-range-declaration is performed by ParseSimpleDeclaration. The
>>> for-range-initializer is parsed there too, in order to ensure that the
>>> for-range-declaration is not in scope in the initializer: "int a =
>>> {1,2,3}; for (auto a : a)" appears to be legal.
>>> 
>>> Most of the semantic analysis for a for-range statement is performed
>>> before the body is parsed, so that the type of the loop variable can be
>>> deduced (in case it contains 'auto'). Attaching the body is performed
>>> as a separate finalization step.
>>> 
>>> For a dependently-typed range expression, much of the semantic analysis
>>> is deferred until template instantiation time. A new flag has been added
>>> to VarDecl to indicate whether it's a for-range-declarator. This flag is
>>> used to skip ActOnUninitializedDecl (specifically, while instantiating a
>>> template containing a dependent for-range statement); such declarators
>>> are never actually uninitialized, but appear that way in the AST if the
>>> required semantic analysis could not be performed.
>>> 
>>> As a small extension, this implementation allows the deduced 'begin'
>>> and 'end' iterators to be of differing types; this allows for-range to
>>> be used for iterators where it's easy to determine whether an iterator
>>> is at the end, but difficult or expensive to construct an explicit
>>> iterator object representing the 'end', or to compare general iterators.
> 
> This extension has been removed: it didn't operate correctly during
> template instantiation.

Okay, thanks!

>>> The wording for for-range statements seems likely to change at the
>>> upcoming WG21 meeting in Madrid (per N3257). Resolution 2 for this
>>> change can be implemented by changing the NeedsADL flag from true to
>>> false in BuildForRangeBeginEndCall in SemaStmt.cpp.
> 
> This patch implements the finalized C++11 wording from N3271.

Excellent. It looks like it came out fairly clean.

>> @@ -1005,6 +1006,328 @@
>> ForLoc, RParenLoc));
>> }
>> 
>> 
>> +namespace {
>> +
>> +/// Build a call to 'begin' or 'end' for a C++0x for-range statement.
>> +/// FIXME: This is likely to be changed as the result of paper N3257.
>> 
>> 
>> We now have final wording for this, which just adds a new step before the
>> ADL-only lookup of begin/end. So I'll just comment on the parts that are
>> the same.
>> 
>> +static ExprResult BuildForRangeBeginEndCall(Sema &SemaRef, Scope *S,
>> +                                            SourceLocation Loc,
>> +                                            const char *Name, Expr *Arg)
>> {
>> +  // Find the appropriate begin or end function.
>> +  IdentifierInfo *II = &SemaRef.PP.getIdentifierTable().get(Name);
>> +  LookupResult R(SemaRef, II, Loc, Sema::LookupOrdinaryName);
>> +
>> +  if (SemaRef.StdNamespace)
>> +    SemaRef.LookupQualifiedName(R, SemaRef.getStdNamespace());
>> 
>> 
>> Qualified name lookup into namespace "std" is not the same as adding
>> "std" as an associated namespace for ADL (there are differences w.r.t.
>> using directives, for example). I suggest modeling the handling of "std"
>> exactly as the working paper specifies.
> 
> This has been fixed. A new flag has been added to UnresolvedLookupExpr to
> add namespace std to the usual set of associated namespaces for the call.

Yep, looks good.

>> +    if (BeginExpr.get()->getType()->isVoidType() ||
>> +        !DeduceAutoType(Context.AutoDeductTy, BeginExpr.get(),
>> BeginType)) {
>> +      Diag(RangeLoc, diag::err_for_range_iter_deduction_failure)
>> +        << BeginExpr.get()->getType();
>> +      NoteForRangeBeginEndFunction(*this, BeginExpr.get(), BEF_begin);
>> +    }
>> 
>> Do we really need the check for 'void' here? Can it be folded into
>> DeduceAutoType, since essentially every user of DeduceAutoType will have
>> to do a 'void' check?
> 
> We don't have to check for 'void' here; we do so in order to provide a
> better diagnostic: "'void' type cannot be used as a range" rather than a
> mysterious error about forming a reference to void.

Okay.

>> As a general note, most of the various expressions and initializers are
>> full expressions, but you don't seem to be calling ActOnFinishFullExpr at
>> all. That will likely manifest as a bug where temporaries created by
>> those expressions won't be destroyed. Which ties in with...
>> 
>> @@ -1175,15 +1201,19 @@
>> if (Body.isInvalid()) return StmtError();
>> 
>> -  if (!ForEach)
>> -    return Actions.ActOnForStmt(ForLoc, LParenLoc, FirstPart.take(),
>> SecondPart,
>> -                                SecondVar, ThirdPart, RParenLoc,
>> Body.take());
>> +  if (ForEach)
>> +    // FIXME: It isn't clear how to communicate the late destruction of
>> +    // C++ temporaries used to create the collection.
>> 
>> 
>> Although this FIXME is specifically written for the Objective-C for
>> collection statement, it seems that the same issue affects the C++0x
>> range-for.  How about gathering the set of temporaries still active after
>> processing the range expression and placing them into the
>> CXXForRangeStmt? CodeGen can then create a special cleanup scope for
>> these temporaries.
> 
> There are quite a few different places where for-range creates
> temporaries, and they have various different lifetime requirements. Most
> of the temporaries were already being cleaned up by AddInitializerToDecl
> (however, some code reordering was required to get the cleanup to pick the
> right temporaries). The two remaining full-expressions, __begin != __end
> and ++__begin, were both missing ActOnFinishFullExpr calls, which I've now
> added.
> 
> I've also added a test to make sure we create and destroy the temporaries
> at the same points as in the corresponding desugaring.

Okay!

>> It would be nice to have a test for the AST read/write of
>> CXXForRangeStmts.
> 
> I've added such a test.
> 
> Please let me know what more needs doing!


I don't see anything else that needs to be done. Please go ahead and commit!

	- Doug