================
Comment at: lib/Sema/Sema.cpp:339
@@ -338,3 +338,3 @@
 ExprResult Sema::ImpCastExprToType(Expr *E, QualType Ty,
                                    CastKind Kind, ExprValueKind VK,
                                    const CXXCastPath *BasePath,
----------------
rsmith wrote:
> Why is `Kind` == `CK_NoOp` being passed in here for an address space cast? 
> That seems like it might be a bug in the caller. Do you know which calls to 
> `ImpCastExprToType` result in this happening?
This is the stack I have when I reach the point where the address cast is 
performed.

```
clang::Sema::ImpCastExprToType() at Sema.cpp:370 0x141629b8     
clang::Sema::PerformImplicitConversion() at SemaExprCXX.cpp:3,447 0x1456981c    
clang::Sema::PerformImplicitConversion() at SemaExprCXX.cpp:2,964 0x14567124    
clang::InitializationSequence::Perform() at SemaInit.cpp:6,315 0x1465e444       
TryStaticImplicitCast() at SemaCast.cpp:1,522 0x141bb004        
TryStaticCast() at SemaCast.cpp:988 0x141b9e98  
(anonymous namespace)::CastOperation::CheckCXXCStyleCast at SemaCast.cpp:2,130 
0x141b4248       
clang::Sema::BuildCStyleCastExpr() at SemaCast.cpp:2,455 0x141b3b60     
clang::Sema::ActOnCastExpr() at SemaExpr.cpp:5,545 0x144533a8   
```

I see CK_NoOp is hardcoded in the caller:

```
  case ICK_Qualification: {
    // The qualification keeps the category of the inner expression, unless the
    // target type isn't a reference.
    ExprValueKind VK = ToType->isReferenceType() ?
                                  From->getValueKind() : VK_RValue;
    From = ImpCastExprToType(From, ToType.getNonLValueExprType(Context),
                             CK_NoOp, VK, /*BasePath=*/nullptr, CCK).get();
```
Should I fix something in here?


================
Comment at: lib/Sema/Sema.cpp:364-370
@@ -363,9 +363,9 @@
 
   if (ImplicitCastExpr *ImpCast = dyn_cast<ImplicitCastExpr>(E)) {
     if (ImpCast->getCastKind() == Kind && (!BasePath || BasePath->empty())) {
       ImpCast->setType(Ty);
       ImpCast->setValueKind(VK);
       return E;
     }
   }
 
----------------
rsmith wrote:
> If we (somehow) hit this case and have an inner cast of kind `CK_NoOp`, we'll 
> fail to create a `CK_AddressSpaceConversion` cast. The added code should be 
> above this block.
Got it. Moved the code up.

================
Comment at: test/Sema/address-space-cast.c:1-2
@@ +1,3 @@
+// RUN: %clang_cc1 %s -ast-dump -x c %s | FileCheck -check-prefix=CHKC %s
+// RUN: %clang_cc1 %s -ast-dump -x c++ %s | FileCheck -check-prefix=CHKCXX %s
+
----------------
rsmith wrote:
> Please use the conventional check prefix of `CHECK` for the first of these, 
> and `CHECK-CXX` for the second.
> 
> Also, it would be preferable to check the generated IR here rather than an 
> AST dump -- the AST dump format is less stable than the textual IR format, 
> and it's much more important that we get the IR right than the dump :-)
Ok, doing now the proper checks!

http://reviews.llvm.org/D7606

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/



_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

Reply via email to