-
Notifications
You must be signed in to change notification settings - Fork 424
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rename div funcs #23130
Rename div funcs #23130
Conversation
…nitions Since we deprecated including them by default in this release cycle, we don't need to deprecate the old versions in Math.chpl, we just need to update them and update the deprecation message in AutoMath to mention the new change we're including ---- Signed-off-by: Lydia Duncan <lydia-duncan@users.noreply.github.com>
Also needed to update the unstable tests to use the new names so they don't trigger the AutoMath versions instead ---- Signed-off-by: Lydia Duncan <lydia-duncan@users.noreply.github.com>
---- Signed-off-by: Lydia Duncan <lydia-duncan@users.noreply.github.com>
---- Signed-off-by: Lydia Duncan <lydia-duncan@users.noreply.github.com>
---- Signed-off-by: Lydia Duncan <lydia-duncan@users.noreply.github.com>
---- Signed-off-by: Lydia Duncan <lydia-duncan@users.noreply.github.com>
Somehow missed committing the submitted mandelbrot adjustments (because they're binary files). Not sure how I managed to miss the verify-div-ceil-floor-mod-1 test, though ---- Signed-off-by: Lydia Duncan <lydia-duncan@users.noreply.github.com>
Signed-off-by: Lydia Duncan <lydia-duncan@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expect the gpu math ops test probably needs to be updated as well, but everything else looks good to me!
---- Signed-off-by: Lydia Duncan <lydia-duncan@users.noreply.github.com>
@@ -1142,7 +1142,7 @@ module AutoMath { | |||
fewer conditionals will be evaluated at run time. | |||
*/ | |||
pragma "last resort" | |||
@deprecated(notes="In an upcoming release 'divceil' will no longer be included by default, please 'use' or 'import' the :mod:`Math` module to call it. Additionally its argument names will also change with this move") | |||
@deprecated(notes="In an upcoming release 'divceil' will no longer be included by default, please 'use' or 'import' the :mod:`Math` module to call it. Additionally its name and argument names will also change with this move") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lydia-duncan : In putting together #23146, I found this family of error messages confusing (even as a pretty savvy user) because the tests in question already had a use Math;
so it didn't sound like I should need to change anything. Because the message refers to "an upcoming release" before saying "will also change", it suggested to me that the name hadn't changed yet / that I didn't need to change the name in addition to adding use Math;
.
To me, it would read more clearly if the wording were more like "In an upcoming release, 'divceil' will no longer be available by default. Please 'use' or 'import' the :mod:Math
module to prepare for this move, noting that its name has changed to 'divCeil()'."
or possibly ", noting that its signature has changed to 'divCeil([new argument names and types here])`"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'll definitely do that rewording (though I suspect the confusion will be less for users since the use Math;
won't necessarily already be present, since their default inclusion was deprecated during this release cycle).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it won't necessarily already be, but in cases like this where it is, it was pretty confusing. Like "You're telling me to do this, but I've already done it, so I'm skipping that sentence. And now you're telling me it's going to change names at some point, but it's still complaining about the symbol today." My first instinct was that we had a bug in our deprecation logic since the code had already done what seemed to be suggested.
Renamed the following functions to follow our capitalization scheme:
Resolves #19012
Since the version of these functions that was included in all
programs by default was deprecated during this release cycle,
I've merely updated the version in Math.chpl for the new names and
adjusted the deprecation message in AutoMath.chpl to mention
the renaming of the function in addition to the arguments.
Updated several standard and package modules to use the new
names.
Updated the documentation links in Math.chpl to link to the new names.
Updated the deprecation message tests for the new expected output
and the unstable tests for divCeilPos and divFloorPos to use the
new names (so they don't trigger the deprecation warnings instead).
Updated several tests to use the new names. For the submitted shootouts,
updated the expected deprecation message for the change, as we want
those tests to stay in line with the version on the site.
Passed a full paratest with futures