aaron.ballman closed this revision.
aaron.ballman added a comment.
Thank you for the new functionality, I've committed on your behalf in
8680f951c21e675a110e79c9b8dc59bb94290b01
<https://reviews.llvm.org/rG8680f951c21e675a110e79c9b8dc59bb94290b01>
================
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;
----------------
junaire wrote:
> 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!
>
> Hi, Aaron! Thanks for your review! Unfortunately, I don't get what you mean.
> :( Sorry about that, I'm completely a newbie...
No apologies necessary, we've all started there! :-)
> 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!
The way you implemented it and the way I suggested it are functionally the same
outcome.
`PrepareBuiltinElementwiseMathOneArgCall()` eventually calls
`TheCall->setType(TyA);` to set the return type of the call expression to be
the same as the argument type. After calling that method, you can either do
`QualType ArgTy = TheCall->getArg(0)->getType();` as you were doing or do
`QualType ArgTy = TheCall->getType();` to get that adjusted type.
The way you've written it currently is correct and likely a more readable
solution. My suggestion would have ever-so-slightly better performance because
it doesn't call `getArg(0)`, but I think we should prefer your approach because
it's more readable. That's why I accepted the patch as-is.
Does that clarify?
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
