Copilot commented on code in PR #2552:
URL: https://github.com/apache/groovy/pull/2552#discussion_r3291074293
##########
src/main/java/org/codehaus/groovy/runtime/DefaultGroovyMethods.java:
##########
@@ -2022,6 +2022,56 @@ public void remove() {
}
}
+
//--------------------------------------------------------------------------
+ // clamp
+
+ /**
+ * Constrains a Comparable value to lie within the inclusive range
+ * <code>[lower, upper]</code>. Returns <code>lower</code> if
<code>self</code>
+ * is less than <code>lower</code>, <code>upper</code> if
<code>self</code> is
+ * greater than <code>upper</code>, otherwise <code>self</code>.
+ * <pre class="language-groovy groovyTestCase">
+ * assert 10.clamp(1, 15) == 10
+ * assert 10.clamp(1, 5) == 5
+ * assert 10.clamp(12, 20) == 12
+ * assert 'a'.clamp('b', 'z') == 'b'
+ * </pre>
+ *
+ * @param self a Comparable value
+ * @param lower the inclusive lower bound
+ * @param upper the inclusive upper bound
+ * @return the clamped value
+ * @throws IllegalArgumentException if <code>lower</code> is greater than
<code>upper</code>
+ * @since 6.0.0
+ */
+ public static <T extends Comparable<? super T>> T clamp(T self, T lower, T
upper) {
+ if (ScriptBytecodeAdapter.compareGreaterThan(lower, upper)) {
+ throw new IllegalArgumentException("lower bound (" + lower + ")
must not be greater than upper bound (" + upper + ")");
+ }
+ if (ScriptBytecodeAdapter.compareLessThan(self, lower)) return lower;
+ if (ScriptBytecodeAdapter.compareGreaterThan(self, upper)) return
upper;
+ return self;
+ }
+
+ /**
+ * Constrains a Comparable value to lie within the given Range.
+ * <pre class="language-groovy groovyTestCase">
+ * assert 10.clamp(12..20) == 12
+ * assert 10.clamp(1..5) == 5
+ * </pre>
Review Comment:
The inline `groovyTestCase` examples don't currently cover reverse ranges or
exclusive-right ranges; with the current implementation, reverse ranges would
fail (and even after fixing, adding assertions helps prevent regressions).
Consider adding assertions like `10.clamp(20..12) == 12` and `10.clamp(1..<5)
== 4`.
##########
src/main/java/org/codehaus/groovy/runtime/DefaultGroovyMethods.java:
##########
@@ -2022,6 +2022,56 @@ public void remove() {
}
}
+
//--------------------------------------------------------------------------
+ // clamp
+
+ /**
+ * Constrains a Comparable value to lie within the inclusive range
+ * <code>[lower, upper]</code>. Returns <code>lower</code> if
<code>self</code>
+ * is less than <code>lower</code>, <code>upper</code> if
<code>self</code> is
+ * greater than <code>upper</code>, otherwise <code>self</code>.
+ * <pre class="language-groovy groovyTestCase">
+ * assert 10.clamp(1, 15) == 10
+ * assert 10.clamp(1, 5) == 5
+ * assert 10.clamp(12, 20) == 12
+ * assert 'a'.clamp('b', 'z') == 'b'
+ * </pre>
+ *
+ * @param self a Comparable value
+ * @param lower the inclusive lower bound
+ * @param upper the inclusive upper bound
+ * @return the clamped value
+ * @throws IllegalArgumentException if <code>lower</code> is greater than
<code>upper</code>
+ * @since 6.0.0
+ */
+ public static <T extends Comparable<? super T>> T clamp(T self, T lower, T
upper) {
+ if (ScriptBytecodeAdapter.compareGreaterThan(lower, upper)) {
+ throw new IllegalArgumentException("lower bound (" + lower + ")
must not be greater than upper bound (" + upper + ")");
+ }
+ if (ScriptBytecodeAdapter.compareLessThan(self, lower)) return lower;
+ if (ScriptBytecodeAdapter.compareGreaterThan(self, upper)) return
upper;
+ return self;
+ }
+
+ /**
+ * Constrains a Comparable value to lie within the given Range.
+ * <pre class="language-groovy groovyTestCase">
+ * assert 10.clamp(12..20) == 12
+ * assert 10.clamp(1..5) == 5
+ * </pre>
+ *
+ * @param self a Comparable value
+ * @param range a Range defining the inclusive bounds
+ * @return the clamped value
+ * @since 6.0.0
+ */
+ @SuppressWarnings("unchecked")
+ public static <T extends Comparable<? super T>> T clamp(T self, Range<T>
range) {
+ T lower = range.isReverse() ? (T) range.getTo() : (T) range.getFrom();
+ T upper = range.isReverse() ? (T) range.getFrom() : (T) range.getTo();
Review Comment:
`Range#getFrom()`/`getTo()` are documented as the lower/upper bounds (and
`ObjectRange` normalizes so `from <= to` even when `isReverse()` is true).
Swapping bounds based on `range.isReverse()` inverts them for reverse ranges
(e.g. `5..1` or `'z'..'a'`), causing `clamp(self, lower, upper)` to throw
`IllegalArgumentException` even though the range is valid. Use
`range.getFrom()` as the lower bound and `range.getTo()` as the upper bound
regardless of `isReverse()`, and this should also let you drop the unchecked
casts and `@SuppressWarnings`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
