Anastasia marked 3 inline comments as done.
Anastasia added inline comments.
================
Comment at: clang/test/SemaOpenCLCXX/addrspace_cast.cl:19-24
+template <class T>
+void test_temp(__global int *par) {
+ T *var1 = addrspace_cast<T *>(par);
+ __private T *var2 = addrspace_cast<__private T *>(par);
+ T var3 = addrspace_cast<T>(par);
+}
----------------
mantognini wrote:
> Does this test anything since it's not instantiated?
It tests AST creation and Sema but not after the TreeTransforms. I have now
enhanced it.
================
Comment at: lib/Sema/SemaCast.cpp:285
+ return Op.complete(CXXAddrspaceCastExpr::Create(Context, Op.ResultType,
+ Op.ValueKind, Op.SrcExpr.get(),
+ DestTInfo,
----------------
mantognini wrote:
> Anastasia wrote:
> > mantognini wrote:
> > > The formatting looks a bit funny here.
> > I agree, I just matched the style of the old code to keep it coherent.
> > Although perhaps I should rather adhere to the current style?
> I don't have a strong opinion on what's best.
I looked at the other review and I think the preference is to stay with the
coding style for the new added code.
================
Comment at: lib/Sema/SemaCast.cpp:2338
auto DestPointeeType = DestPtrType->getPointeeType();
if (SrcPointeeType.getAddressSpace() == DestPointeeType.getAddressSpace())
return TC_NotApplicable;
----------------
mantognini wrote:
> Anastasia wrote:
> > mantognini wrote:
> > > Wouldn't this limit usage of the cast unnecessarily? I'm thinking this
> > > could be transformed to a NOP, which could be beneficial to make
> > > (template) code simpler to write.
> > I am not sure what you mean. I have added the test for templates and it
> > caught a bug in lib/AST/Expr.cpp with assert condition.
> >
> > However, now that I think about this more, I believe we should allow
> > compiling this?
> >
> > ```
> > __private int* i;
> > addrspace_cast<private int*>(i);
> > ```
> >
> > Currently it outputs an error.
> Yes, that's what I meant. (Although I see it's not part of this review so I'm
> not saying this should be changed now.)
It was an easy change so I have updated the patch. :)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60193/new/
https://reviews.llvm.org/D60193
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
