fhahn marked 6 inline comments as done. fhahn added inline comments.
================ Comment at: clang/lib/Sema/SemaChecking.cpp:16673-16678 + Expr *A = TheCall->getArg(0); + Expr *B = TheCall->getArg(1); + QualType TyA = A->getType(); + QualType TyB = B->getType(); + + if (TyA != TyB) ---------------- aaron.ballman wrote: > fhahn wrote: > > aaron.ballman wrote: > > > Should these arguments undergo usual conversions (array to pointer decay, > > > integer promotions, etc)? > > I intentionally went with not performing conversions, because it might lead > > to surprising results. If the arguments have different types, it is not > > clear to me which type should be chosen to try convert the other one; e.g. > > if we have __builtin_elementwise_max(float, int)` should the first argument > > be converted to `int` or the second one to `float`? > > > > Forcing the types to match without conversion seem to make them less > > error-prone to use, but I might be missing some general rule to handle type > > conflicts here? > I'm not certain how these builtins are expected to be used, but if they're > likely to be used with literals, I think we may want integer promotions > because of that alone. > > Btw, given that these only have two arguments, it seems like we could use the > usual arithmetic conversions to pick a common type the same way as happens > for binary operators. > > If you end up not adding any conversions here, we should have a test case to > cover passing in two array objects (which won't decay to pointers). > I'm not certain how these builtins are expected to be used, but if they're > likely to be used with literals, I think we may want integer promotions > because of that alone. Yes, they should ideally work with literals! I think it should be relatively straight-forward to add promotion of literals. > Btw, given that these only have two arguments, it seems like we could use the > usual arithmetic conversions to pick a common type the same way as happens > for binary operators. IIUC this happens in `SemaOverload.cpp`, but I am not sure how builtins would hook into the infrastructure there. Are there other builtins that are similarly overloaded by any chance? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111985/new/ https://reviews.llvm.org/D111985 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
