junaire added a comment. In D114688#3176567 <https://reviews.llvm.org/D114688#3176567>, @aaron.ballman wrote:
> In D114688#3176433 <https://reviews.llvm.org/D114688#3176433>, @fhahn wrote: > >> I think it might be good to split off the refactoring of >> `SemaBuiltinElementwiseMathOneArg`-> >> `PrepareBuiltinElementwiseMathOneArgCall` and the update for >> `BI__builtin_elementwise_abs` into a separate, non functional change (NFC). > > FWIW, I would not be opposed to that, but I don't insist either. LGTM! Hi, @fhahn @aaron.ballman . Thanks for your patient reviews and valuable suggestions! I think I might not apply the extra suggestion as I don't really understand that, sorry... BTW, if you think this patch is ready, could you please commit this for me? You can use: Jun Zhang j...@junz.org Cheers ================ Comment at: clang/lib/Sema/SemaChecking.cpp:16727-16728 QualType TyA = A.get()->getType(); - if (checkMathBuiltinElementType(*this, ArgLoc, TyA)) + if (checkMathBuiltinElementType(*this, ArgLoc, TyA) || + ExtraCheck(TyA, ArgLoc)) return true; ---------------- fhahn wrote: > aaron.ballman wrote: > > In some ways, it's a not really ideal that we split the type checking like > > this. `SemaBuiltinElementwiseMathOneArg()` checks some type validity and > > the caller checks some more type validity, so there's no one location to > > know what types are actually expected for any given builtin calling this. > > > > Would it be reasonable to instead hoist all the type checking code out of > > `SemaBuiltinElementwiseMathOneArg()` and not pass in a functor to it, but > > instead check the `CallExpr` type after the call returns? We could also fix > > up the function name to be a bit less nonsensical at the same time, like > > renaming it to `PrepareBuiltinElementwiseMathOneArgCall()` or something > > along those lines? > > Would it be reasonable to instead hoist all the type checking code out of > > SemaBuiltinElementwiseMathOneArg() and not pass in a functor to it, but > > instead check the CallExpr type after the call returns? > > Yeah that sounds better! > In some ways, it's not really ideal that we split the type checking like > this. `SemaBuiltinElementwiseMathOneArg()` checks some type validity and the > caller checks some more type validity, so there's no one location to know > what types are actually expected for any given builtin calling this. > > Would it be reasonable to instead hoist all the type checking code out of > `SemaBuiltinElementwiseMathOneArg()` and not pass in a functor to it, but > instead check the `CallExpr` type after the call returns? We could also fix > up the function name to be a bit less nonsensical at the same time, like > renaming it to `PrepareBuiltinElementwiseMathOneArgCall()` or something along > those lines? Hi, Aaron! Thanks for your review! Unfortunately, I don't get what you mean. :( Sorry about that, I'm completely a newbie... >check the `CallExpr` type after the call returns? What does this mean? From my understanding, we only need to check the argument type, right? Or do you mean something like: ``` TheCall->setType(TyA); // inside checking function ... TheCall->getType(); // call side ``` Do I understand you correctly? Please don't hesitate to point out any mistake I made, thanks! ================ Comment at: clang/lib/Sema/SemaChecking.cpp:16721 +bool Sema::SemaBuiltinElementwiseMathFloatArg(CallExpr *TheCall) { + if (checkArgCount(*this, TheCall, 1)) ---------------- fhahn wrote: > If I understand correctly, this is similar to > `SemaBuiltinElementwiseMathOneArg`, but with the additional restriction to > only allow floating point element types? > > Do you think it would be possible to extend > `SemaBuiltinElementwiseMathOneArg` to handle this case directly, without > needing to duplicate most of the other logic? > If I understand correctly, this is similar to > `SemaBuiltinElementwiseMathOneArg`, but with the additional restriction to > only allow floating point element types? > > Do you think it would be possible to extend > `SemaBuiltinElementwiseMathOneArg` to handle this case directly, without > needing to duplicate most of the other logic? Hi, Florian, Thanks for your suggestion! I have updated the patch, PTAL :-) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114688/new/ https://reviews.llvm.org/D114688 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
