[SYCL][NFC] Diagnostic message improvement for negative arguments values of work_group_size attributes #2988
Conversation
…s of work_group_size attributes Template parameter support for work_group_size attributes was added on intel#2906 This is a followup comments on intel#2906 This patch improves the diagnostic message for "warn_attribute_requires_non_negative_integer_argument" and updates tests related to this diagnostic. Signed-off-by: Soumi Manna <soumi.manna@intel.com>
Signed-off-by: Soumi Manna <soumi.manna@intel.com>
smanna12
changed the title
smanna12
requested review from
elizabethandrews,
Fznamznon and
premanandrao
as code owners
clang/include/clang/Sema/Sema.h
Outdated
| @@ -12981,7 +12981,8 @@ static bool handleMaxWorkSizeAttrExpr(Sema &S, const AttrInfo &AI, | |||
| if (ArgVal->isNegative()) { | |||
| S.Diag(Expr->getExprLoc(), | |||
| diag::warn_attribute_requires_non_negative_integer_argument) | |||
| << &AI << Idx << Expr->getSourceRange(); | |||
| << S.Context.IntTy << S.Context.UnsignedIntTy | |||
There was a problem hiding this comment.
I think you should get the type out of the expression rather than hard-code IntTy -- the user could have done something like used an integer suffix that would give a different type, and the diagnostic would be somewhat confusing. e.g., 0LL instead of 0.
You should use UnsignedLongLongTy for the unsigned type as that's what we always wind up converting it to anyway.
There was a problem hiding this comment.
Thanks @AaronBallman for lookin into the patch.
If i understand correctly, do you mean to use something like << S.Context.IntTy << S.Context.UnsignedLongLongTy
I think you should get the type out of the expression rather than hard-code IntTy --
I will take a look at this and try to do this way.
There was a problem hiding this comment.
Nope, sorry for not being more clear. I mean something more like << Expr->getType() << S.Context.UnsignedLongLongTy
(as a drive-by, we may want to rename the variable Expr in this function to something that doesn't conflict with the type named Expr.)
There was a problem hiding this comment.
Thanks for the explanation. Yes, it makes sense to me to use UnsignedLongLongTy for the unsigned type. I have renamed he variable Expr to avoid the conflicts with type names Expr.
Signed-off-by: Soumi Manna <soumi.manna@intel.com>
|
Thank you @elizabethandrews and @AaronBallman for the reviews. |
Template parameter support for work_group_size attributes was added on
#2906
This is a follow-up comments in #2906 (comment)
This patch improves the diagnostic message for
"warn_attribute_requires_non_negative_integer_argument" and updates tests
related to this diagnostic.
Signed-off-by: Soumi Manna soumi.manna@intel.com