tra accepted this revision. tra added a comment. LGTM in general, modulo a readability suggestion.
================ Comment at: clang/lib/Sema/SemaExpr.cpp:6548-6550 + // Prevent addrspace cast if the parameter has a default address + // space, or the argument has a non-default addrspace and the + // target addrspaces of the argument and the parameter differ. ---------------- tra wrote: > I'd rephrase it in terms of `add ASC if target AS is different`. > > `Prevent` assumes that we've already decided to add an ASC, while it is not > the case. All we know is that we've got a pointer argument and we are > figuring out whether ASC is needed. Hmm. I guess that does not quite match what we do here. Oringal code would add ASC for generic->specific only. The test cases you've addedd are for specific->specific AS, AFAICT, and that makes me wonder if I understand what's going on here. So the test case currently does fail, because we end up w/o implicit ASC and end up trying to pass wrong types of pointers. https://godbolt.org/z/MaWMbGb5z Now we actually want to allow implicit ASC for the pointer types that are in the *same* target AS, so my comment suggestion was actually wrong. This confusion also suggests that the condition is hard to understand. Perhaps we can rewrite it. E.g. ``` bool NeedImplicitASC = ParamAS != LangAS::Default && // Pointers params in generic AS don't need special handling. ( ArgAs == LangAS::Default || // We do allow implicit conversion from generic AS // or from specific AS which has target AS matching that of Param. getASTContext().getTargetAddressSpace(ArgAS) == getASTContext().getTargetAddressSpace(ParamAS)); if (!NeedImplicitASC) continue; ``` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111734/new/ https://reviews.llvm.org/D111734 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
