arichardson planned changes to this revision.
arichardson added inline comments.


================
Comment at: include/clang/Basic/AddressSpaces.h:66
 
+inline LangAS LangASFromTargetAS(unsigned TargetAS) {
+  return static_cast<LangAS>((TargetAS) +
----------------
yaxunl wrote:
> how about `getLangASFromTargetAS` ? It is preferred to start with small 
> letters.
Sounds good, I'll do that.


================
Comment at: tools/libclang/CXType.cpp:402
+  ASTContext &Ctx = cxtu::getASTUnit(GetTU(CT))->getASTContext();
+  return Ctx.getTargetAddressSpace(T);
 }
----------------
yaxunl wrote:
> Is this function suppose to return AST address space or target address space?
> 
> Some targets e.g. x86 maps all AST address spaces to 0. Returning target 
> address space will not let the client unable to differentiate different 
> address spaces in the source language.
I am not entirely sure what the correct return value is here because the 
current implementation returns either the LanguageAS or `LangAS - 
LangAS::FirstTargetAddressSpace` which can also overlap. So possibly it should 
just always returning the AST address space?

I think for now I will just keep the current behaviour with a FIXME and create 
a followup patch.



https://reviews.llvm.org/D38816



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to