Anastasia marked an inline comment as done. Anastasia added inline comments.
================ Comment at: lib/Sema/SemaCast.cpp:2309 + auto DestPointeeTypeWithoutAS = Self.Context.removeAddrSpaceQualType( + DestPointeeType.getCanonicalType()); + return Self.Context.hasSameType(SrcPointeeTypeWithoutAS, ---------------- rjmccall wrote: > ebevhan wrote: > > Anastasia wrote: > > > Anastasia wrote: > > > > ebevhan wrote: > > > > > Maybe I'm mistaken, but won't getting the canonical type here drop > > > > > qualifiers (like cv) in nested pointers? If so, an addrspace_cast > > > > > might strip qualifiers by mistake. > > > > Yes, indeed I will need to extend this to nested pointers when we are > > > > ready. But for now I can try to change this bit... however I am not > > > > sure it will work w/o canonical types when we have typedef. I will try > > > > to create an example and see. > > > I checked the canonical type does preserve the qualifiers correctly. > > > > > > Here is the AST dump of the following C type `mytype const __generic*` > > > where `typedef __generic int* mytype;`. > > > > > > > > > ``` > > > PointerType 0x204d3b0 'const __generic mytype *' > > > `-QualType 0x204d369 'const __generic mytype' const __generic > > > `-TypedefType 0x204d320 'mytype' sugar > > > |-Typedef 0x204d1b0 'mytype' > > > `-PointerType 0x204d170 '__generic int *' > > > `-QualType 0x204d158 '__generic int' __generic > > > `-BuiltinType 0x2009750 'int' > > > ``` > > > > > > and it's canonical representation in AST is: > > > > > > ``` > > > PointerType 0x204d380 '__generic int *const __generic *' > > > `-QualType 0x204d349 '__generic int *const __generic' const __generic > > > `-PointerType 0x204d170 '__generic int *' > > > `-QualType 0x204d158 '__generic int' __generic > > > `-BuiltinType 0x2009750 'int' > > > ``` > > > > > > So using canonical type will just simply handling of nested pointer chain > > > by avoiding special casing typedefs. We won't loose any qualifiers. > > Okay, seems good then! > It seems to me that the rules in this function are the reasonable > cross-language rules. If you want to check for OpenCL at the top as a > fast-path check (taking advantage of that fact that no other language modes > currently have overlapping address spaces), that might be reasonable, but the > comment and indentation should reflect that. Btw, I attempted to remove the OpenCL check and unfortunately found failure in the following test: **test/CodeGenCXX/address-space-cast.cpp** ``` error: C-style cast from 'char *' to '__attribute__((address_space(5))) char *' converts between mismatching address spaces __private__ char *priv_char_ptr = (__private__ char *)gen_char_ptr; ``` I am not sure what such conversion does but I feel if I want to update it I might need to get wider agreement. I think sending an RFC might be a good way forward. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58346/new/ https://reviews.llvm.org/D58346 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
