Fixes for References

Hi,

This is an updated patch to fix references. It now should implement subscripted references and references to functions correctly. I believe I got all of Chris's feedback into this patch. And it has the benefit of actually working!! :slight_smile:

Okay to commit?

-bw

diff.txt (4.96 KB)

Heh, oops, I just responded.

This patch has similar problems to the previous one. The citation [C++ 3.10p5] doesn't make sense for isLvalue, because it only applies to call results. isLvalue applies to everything.

+++ Sema/SemaExpr.cpp (working copy)
@@ -282,10 +282,8 @@
    Expr *LHSExp = static_cast<Expr*>(Base), *RHSExp = static_cast<Expr*>(Idx);

    // Perform default conversions.
- DefaultFunctionArrayConversion(LHSExp);
- DefaultFunctionArrayConversion(RHSExp);

Hi,

This is an updated patch to fix references. It now should implement subscripted references and references to functions correctly. I believe I got all of Chris's feedback into this patch. And it has the benefit of actually working!! :slight_smile:

Heh, oops, I just responded.

We crossed pathes :slight_smile:

This patch has similar problems to the previous one. The citation [C++ 3.10p5] doesn't make sense for isLvalue, because it only applies to call results. isLvalue applies to everything.

+++ Sema/SemaExpr.cpp (working copy)
@@ -282,10 +282,8 @@
   Expr *LHSExp = static_cast<Expr*>(Base), *RHSExp = static_cast<Expr*>(Idx);

   // Perform default conversions.
- DefaultFunctionArrayConversion(LHSExp);
- DefaultFunctionArrayConversion(RHSExp);
-
- QualType LHSTy = LHSExp->getType(), RHSTy = RHSExp->getType();
+ QualType LHSTy = DefaultFunctionArrayConversion(LHSExp);
+ QualType RHSTy = DefaultFunctionArrayConversion(RHSExp);
   assert(!LHSTy.isNull() && !RHSTy.isNull() && "missing types");

This is logically independent from the rest of the patch. I'd like to eventually remove the return value of DefaultFunctionArrayConversion, so please remove this piece.

That would break the references stuff. The "stripping of the references" is why I made this change. Otherwise, we get a reference type from

   QualType LHSTy = LHSExp->getType(), RHSTy = RHSExp->getType();

And it fails below. If you're going to remove the return value of DefaultFunctionArrayConversion, then we'd have to do yet another stripping of the reference afterwards.

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

This spec citation also seems wrong. It applies to initialization of references.

I'll put the correct one on when I fix the bigger problem.

-bw

This is logically independent from the rest of the patch. I'd like to eventually remove the return value of DefaultFunctionArrayConversion, so please remove this piece.

That would break the references stuff. The "stripping of the references" is why I made this change. Otherwise, we get a reference type from

  QualType LHSTy = LHSExp->getType(), RHSTy = RHSExp->getType();

And it fails below. If you're going to remove the return value of DefaultFunctionArrayConversion, then we'd have to do yet another stripping of the reference afterwards.

That's the problem: after calling DefaultFunctionArrayConversion, the expr returned should have its reference stripped off with an implicit conversion.

One invariant is that the type (current) returned by DefaultFunctionArrayConversion should always be the type of the expr.

-Chris

Then that makes my patch even simpler. Here's the relevant part:

Index: Sema/SemaExpr.cpp

That's the problem: after calling DefaultFunctionArrayConversion, the expr returned should have its reference stripped off with an implicit conversion.

One invariant is that the type (current) returned by DefaultFunctionArrayConversion should always be the type of the expr.

Then that makes my patch even simpler. Here's the relevant part:

Nice, now we're getting there :slight_smile:

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

Plz use a textual spec citation instead of a numeric one [foo.bar]. Other than that, it looks ok, please commit.

It would be nice to refactor the code so that UsualUnaryConversions only needs to check for references once, but I'm not sure if that is possible.

As a follow on patch, please eliminate the call to getCanonicalType by enhancing the Type::isReferenceType() predicate to return the reference type (like we now do for pointer type). This will allow you to retain the typedef info for reference typedefs.

-Chris

Nice, now we're getting there :slight_smile:

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

Plz use a textual spec citation instead of a numeric one [foo.bar].
Other than that, it looks ok, please commit.

Okay.

It would be nice to refactor the code so that UsualUnaryConversions
only needs to check for references once, but I'm not sure if that is
possible.

As a follow on patch, please eliminate the call to getCanonicalType
by enhancing the Type::isReferenceType() predicate to return the
reference type (like we now do for pointer type). This will allow
you to retain the typedef info for reference typedefs.

I'll submit tonight. Thanks! :slight_smile:

-bw