-
Notifications
You must be signed in to change notification settings - Fork 421
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 BoundedRangeType to boundKind, range.boundedType to range.bounds #22059
Conversation
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.
The changes in test/*
look good to me
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.
The changes themselves look good. I haven't read through the test updates but I did see your added deprecation test and would assume if it passes standard and gasnet paratests things should be fine.
modules/internal/ChapelRange.chpl
Outdated
@@ -42,6 +42,11 @@ module ChapelRange { | |||
whereas the new rule reverses such direction. */ | |||
config param newSliceRule = false; | |||
|
|||
// This enum is documented directly in the spec to fit the presentation flow. | |||
pragma "no doc" |
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.
Are both pragma "no doc"
and @chpldoc.nodoc
necessary, don't these do the same thing?
proc BoundedRangeType type do return boundKind; | ||
//was: enum BoundedRangeType { bounded, boundedLow, boundedHigh, boundedNone }; | ||
@deprecated("'bounded' is deprecated; please use 'boundKind.both' instead") | ||
proc type boundKind.bounded param do return boundKind.both; |
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.
Oh very cool, I didn't realize you could add this kind of "alias" of one enum value to another.
// Deprecated by Vass in 1.31: given `use BoundedRangeType`, | ||
// redirect it to `use boundKind`, with a deprecation warning. | ||
// This seems to handle `import BoundedRangeType` as well. | ||
static void checkRangeDeprecations(ResolveScope* scope, UseStmt* use, |
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.
It's kind of unfortunate that we have to add this checking code to the compiler.
In the range module you added this:
proc BoundedRangeType type do return boundKind
But I guess that doesn't fix things for a statement like this:
use BoundedRangeType;
because we don't actually evaluate BoundedRangeType
at compile-time before evaluating the use
statement (so it thinks you're trying to pass a proc to the use
statement). Could we / should we change this behavior? Having to modify compiler code for this case isn't just a pain for us, it also means there's no way for our users to deprecate and rename enums in their own modules.
That said, I don't think I don't think I'd try to generalize things for this PR, but if there isn't already a Github issue on this (e.g. "how can we rename an enum and its members and keep the old one around deprecated without having to modify compiler code") then I'd suggest creating one and having that cite back to this code review.
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.
Good suggestion. I created #22069, with pointers to two prior related issues.
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 don't think the compiler change is necessary - can't you just make the old name a type alias to the new enum and deprecate the type alias?
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.
If you can, it would be great to add a test of this to test/deprecated-keyword
, either extending one of the enum examples or adding a new one. It might also be helpful to add it as a word-around example to https://chapel-lang.org/docs/latest/developer/bestPractices/Deprecation.html#best-practices-deprecation in the meanwhile
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.
can't you just make the old name a type alias to the new enum and deprecate the type alias?
Nice suggestion. Alas this is not supported today:
error: use must name a module or enum ('BoundedRangeType' is neither)
it would be great to add a test of this to test/deprecated-keyword
I consider this throw-away code and do not see a justification to spend any effort on maintaining and testing it.
It might also be helpful to add it as a work-around example
Sure, please review: #22077
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.
The key phrase in that second comment was "if you can", I don't think it's worth checking in a reference to this significant a work-around, especially without a Chapel code example.
Resolves chapel-lang#17126 This PR implements the following renamings and deprecates the old names, per chapel-lang#17126 (comment) field range.boundedType --> range.bounds enum BoundedRangeType --> enum boundKind enum constants: bounded --> both boundedLow --> low boundedHigh --> high boundedNone --> neither Key changes: * support for the redirections in ChapelRange.chpl and the compiler * switch to the new names in: - modules - tests - compiler - spec * add a deprecation test While there: * add future tests for chapel-lang#22042 * remove `test/io/formatters/basic-types.good` because it is unused and is identical to .default.good * simplify copying of maps in a UseStmt constructor * define the terms "bounded range" and "unbounded range" in the spec * other minor tweaks Detailed dev history: e0178ad..03d3bea and acd40ed..66fff36 r: @jabraham17, @stonea --------------------------------- Here is the python script I used to do the bulk of the renamings: import sys, re; filename = sys.argv[1] try: with open(filename, 'r') as fp: lines=fp.read() except Exception as e: print(filename) print(e) sys.exit(1) orig = lines def bb0(m): print(m.group(), "||", m.groups()) return "xxx" def bb(m): if m.group() == "BoundedRangeType": return "boundKind" if m.group(2) == "": return "boundKind.both" if m.group(2) == "Low": return "boundKind.low" if m.group(2) == "High": return "boundKind.high" if m.group(2) == "None": return "boundKind.neither" print(f"bb: unknown <{m.group()}>") sys.exit(1) def rr(m): if m.group(2) == "": return f"{m.group(1)},both," if m.group(2) == "Low": return f"{m.group(1)},low," if m.group(2) == "High": return f"{m.group(1)},high," if m.group(2) == "None": return f"{m.group(1)},neither," print(f"rr: unknown <{m.group()}>") sys.exit(1) lines = re.sub(r'\bBoundedRangeType(.bounded(|Low|High|None)|)\b', bb, lines) lines = re.sub(r'\b(range\(u?int\(\d+\)),bounded(|Low|High|None),',rr, lines) lines = re.sub(r'\bboundedType\b', 'bounds', lines) if lines != orig: print(filename) with open(filename, 'w') as fp: fp.write(lines) --------------------------------- Signed-off-by: Vassily Litvinov <vasslitvinov@users.noreply.github.com>
66fff36
to
d837a65
Compare
Update the Ranges and Domains and Coercions chapters of the spec and the comments in the ranges and other primers for the change `range.stridable` --> `strides: strideKind` in #22441, with follow-ons #22486 and #22508. Also remove the remaining occurrences of `BoundedRangeType` from the spec and primers -- it is deprecated as of #22059. Remove the `const` return and yield intent in the Procedures, Iterators, and Tuples sections of the spec -- it is unstable as of #22515. I did not see any need to update the primers for this change. While there, I did some rephrasing and restructuring of the text for better readability or precision. Post-merge: update the Chapel Evolution document. r: @benharsh
#24136) These features were deprecated in 1.31 as follows: range.`boundedType` and `BoundedRangeType` issue: #17126 implementation: #22059 range.`stridable` issue: #17131 implementation: #22441, #22486, #22508 This PR removes support for these features, which was added in the above PRs, and adjusts modules and tests accordingly. More range/domain deprecated feature removals are next steps. r: @jabraham17
Resolves #17126
This PR implements the following renamings, as per #17126 (comment) and deprecates the old names:
boundedType
bounds
BoundedRangeType
boundKind
bounded
both
boundedLow
low
boundedHigh
high
boundedNone
neither
Key changes:
While there:
test/io/formatters/basic-types.good
because it is unused and is identical to .default.goodclick to see the script I used to do the bulk of the renamings
Detailed dev history: e0178ad..03d3bea and acd40ed..66fff36