Patch for References

Hi all,

Here's a patch to correct some reference problems. We weren't accounting for references to functions and references to arrays. So something like this would produce errors:

int g(int);

void f() {
   int (&rg)(int) = g;
   rg(i); // Error

   int a[3];
   int (&ra)[3] = a;
   ra[1] = i; // Error
}

Okay to commit?

-bw

diff.txt (5.14 KB)

+ if (isa<ReferenceType>(TR.getCanonicalType())) // C++ 3.10p5 references
+ return LV_Valid;

This citation doesn't make sense, it's talking about function results, not references in general.

+ } else if (isa<ReferenceType>(canonT1)) {
+ // C++ 8.5.3p1: A reference to an array.
+ baseType = canonT1;
+ indexType = canonT2;
+ baseExpr = static_cast<Expr *>(Base);
+ indexExpr = static_cast<Expr *>(Idx);
+ } else if (isa<ReferenceType>(canonT2)) { // uncommon
+ // C++ 8.5.3p1: A reference to an array.
+ baseType = canonT2;
+ indexType = canonT1;
+ baseExpr = static_cast<Expr *>(Idx);
+ indexExpr = static_cast<Expr *>(Base);

This doesn't make sense to me. Wouldn't it be incorrect for:

int *Q;
int *&P = Q;

  P[1];

?

-Chris

+ if (isa<ReferenceType>(TR.getCanonicalType())) // C++ 3.10p5 references
+ return LV_Valid;

This citation doesn't make sense, it's talking about function results, not references in general.

True. I'll come up with a better citation. (The reason that one was there is because I found this error while trying to compile a call to a function that returns a reference.)

+ } else if (isa<ReferenceType>(canonT1)) {
+ // C++ 8.5.3p1: A reference to an array.
+ baseType = canonT1;
+ indexType = canonT2;
+ baseExpr = static_cast<Expr *>(Base);
+ indexExpr = static_cast<Expr *>(Idx);
+ } else if (isa<ReferenceType>(canonT2)) { // uncommon
+ // C++ 8.5.3p1: A reference to an array.
+ baseType = canonT2;
+ indexType = canonT1;
+ baseExpr = static_cast<Expr *>(Idx);
+ indexExpr = static_cast<Expr *>(Base);

This doesn't make sense to me. Wouldn't it be incorrect for:

int *Q;
int *&P = Q;

P[1];

?

True. How about this?

Index: SemaExpr.cpp

True. How about this?

I still don't think this is right. Nothing in the subscript operator should care about arrays. You should rely on the usual unary conversions for decaying the array to a pointer. Perhaps you need to implement usual unary conversions for references?

-Chris

Ah! I understand what you're saying now. Here's a new patch submission. I think it does the correct thing now. I even have the correct standard citation. :slight_smile:

-bw

diff.txt (4.29 KB)

This patch has some of the same problems as the previous one. For example:

This patch has some of the same problems as the previous one. For example:

+
+ const FunctionType *funcT = 0;
+
+ if (PT == 0) {
+ // C++ 8.5.3p1: This is a reference to a function.
+ const ReferenceType *RT = dyn_cast<ReferenceType>(qType);
+
+ if (RT) {
+ funcT = dyn_cast<FunctionType>(RT->getReferenceeType());
+
+ if (funcT == 0)
+ funcT =

...

The reference should be stripped off automatically by "implicit conversion handling" (See [conv], S4p3), not with explicit code in the call expr handler, because the callee is an rvalue.

Okay.

+ if (const ReferenceType *ref = dyn_cast<ReferenceType>(t.getCanonicalType()))
+ t = ref->getReferenceeType(); // C++ 5p5

5p5 talks about the pointer-to-member operator, so I'm not sure how it is relevant.

Mine doesn't talk about pointer-to-member operators. It is off by one, but not that far off (see below). :slight_smile:

When citing the C++ standard, please use the section name as well, such as "[expr.mptr.oper]". I assume the intent is for these names to be stable as the standard moves forward.

Off by one error. It's 5p6:

If an expression initially has the type “reference toT” (8.3.2, 8.5.3), the type is adjusted to “T” prior to any further analysis, the expression designates the object or function denoted by the reference, and the expression is an lvalue.

It only has "[expr]" as the section (chapter) name.

-Expr::isLvalueResult Expr::isLvalue() {
+Expr::isLvalueResult Expr::isLvalue() const {

Marking this method const is fine, please commit it as a separate patch.

Sure.

+ if (isa<ReferenceType>(TR.getCanonicalType())) // C++ 5p5
+ return LV_Valid;

Likewise, this citation is non-sensical, see chapter 4 for the cite you are looking for.

It's 5p6 :slight_smile:

-bw

Actually, even better, the fix for the subscripting thingy works for this as well. So there isn't any modification necessary. :slight_smile:

I'll give you a submission soon.

-bw

When citing the C++ standard, please use the section name as well, such as "[expr.mptr.oper]". I assume the intent is for these names to be stable as the standard moves forward.

Off by one error. It's 5p6:

If an expression initially has the type “reference toT” (8.3.2, 8.5.3), the type is adjusted to “T” prior to any further analysis, the expression designates the object or function denoted by the reference, and the expression is an lvalue.

It only has "[expr]" as the section (chapter) name.

Ah, yep, we both made a mistake. You were off by one, I was looking at 5.5, not 5p5 :).

5p6 makes sense, this should go in unary conversions.

-Chris

Yes, the section names will remain the same but we have already changed some section and paragraph numbers.

  - Doug