From 5b46f42b7dfc98858642d93567db740ed8eceeaf Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Tue, 27 Aug 2024 10:45:23 -0700 Subject: [PATCH 01/22] add the relativeComparator Signed-off-by: Jade Abraham --- modules/standard/Sort.chpl | 148 ++++++++++++++++++++++++++----------- 1 file changed, 104 insertions(+), 44 deletions(-) diff --git a/modules/standard/Sort.chpl b/modules/standard/Sort.chpl index 08831217f129..ef11fed5a1a8 100644 --- a/modules/standard/Sort.chpl +++ b/modules/standard/Sort.chpl @@ -341,7 +341,7 @@ inline proc chpl_compare(a:?t, b:t, comparator:?rec) { comparator.key(b)); // Use comparator.compare(a, b) if is defined by user } else if canResolveMethod(comparator, "compare", a, b) { - return comparator.compare(a ,b); + return comparator.compare(a, b); } else if canResolveMethod(comparator, "chpl_keyPartInternal", a, 0) || canResolveMethod(comparator, "keyPart", a, 0) { return compareByPart(a, b, comparator); @@ -352,54 +352,17 @@ inline proc chpl_compare(a:?t, b:t, comparator:?rec) { } +// helper for chpl_check_comparator, should not be called elsewhere pragma "unsafe" // due to 'data' default-initialized to nil for class types -/* - Check if a comparator was passed and confirm that it will work, otherwise - throw a compile-time error. - - :arg a: Sample data passed to confirm that comparator methods can resolve - :arg comparator: :ref:`Comparator ` record that defines how the - data is sorted. - - */ -proc chpl_check_comparator(comparator, - type eltType, - param errorDepth = 2, - param doDeprecationCheck = true) param { +proc chpl_check_comparator_keyPart(comparator, + type eltType, + param errorDepth = 2, + param doDeprecationCheck = true) param { // Dummy data for checking method resolution // This may need updating when constructors support non-default args const data: eltType; - if comparator.type == DefaultComparator {} - else if isSubtype(comparator.type, ReverseComparator) { - return chpl_check_comparator(comparator.comparator, eltType, errorDepth+1); - } - // Check for valid comparator methods - else if canResolveMethod(comparator, "key", data) { - // TODO: check for keyComparator - // Check return type of key - const keydata = comparator.key(data); - type keytype = keydata.type; - if !(canResolve("<", keydata, keydata)) then - compilerError(errorDepth=errorDepth, "The key method in ", comparator.type:string, " must return an object that supports the '<' function when used with ", eltType:string, " elements"); - - // TODO: this should check that multiple interfaces are not implemented as well - // Check that there isn't also a compare or keyPart - if canResolveMethod(comparator, "compare", data, data) { - compilerError(errorDepth=errorDepth, comparator.type:string, " contains both a key method and a compare method"); - } - if canResolveMethod(comparator, "keyPart", data, 0) { - compilerError(errorDepth=errorDepth, comparator.type:string, " contains both a key method and a keyPart method"); - } - } - else if canResolveMethod(comparator, "compare", data, data) { - // TODO: check for keyPartComparator or relativeComparator - // Check return type of compare - type comparetype = comparator.compare(data, data).type; - if !(isNumericType(comparetype)) then - compilerError(errorDepth=errorDepth, "The compare method in ", comparator.type:string, " must return a numeric type when used with ", eltType:string, " elements"); - } - else if canResolveMethod(comparator, "chpl_keyPartInternal") { + if canResolveMethod(comparator, "chpl_keyPartInternal", data, 0) { var idx: int = 0; type partType = comparator.chpl_keyPartInternal(data, idx).type; if !isTupleType(partType) then @@ -450,7 +413,90 @@ proc chpl_check_comparator(comparator, compilerError(errorDepth=errorDepth, "The keyPart method in ", comparator.type:string, " must return a tuple with element 1 of type int(?) or uint(?) when used with ", eltType:string, " elements"); } } +} + +pragma "unsafe" // due to 'data' default-initialized to nil for class types +/* + Check if a comparator was passed and confirm that it will work, otherwise + throw a compile-time error. + + :arg a: Sample data passed to confirm that comparator methods can resolve + :arg comparator: :ref:`Comparator ` record that defines how the + data is sorted. + + */ +proc chpl_check_comparator(comparator, + type eltType, + param errorDepth = 2, + param doDeprecationCheck = true) param { + + // TODO: add keyComparator + // if more than 1 interface is implemented, error + if (comparatorImplementsKeyPart(comparator):int + + comparatorImplementsRelative(comparator):int) > 1 { + compilerError(errorDepth=errorDepth, "The comparator " + comparator.type:string + " should only implement one sort comparator interface."); + } + + // Dummy data for checking method resolution + // This may need updating when constructors support non-default args + const data: eltType; + + if comparator.type == DefaultComparator {} + else if isSubtype(comparator.type, ReverseComparator) { + return chpl_check_comparator(comparator.comparator, eltType, errorDepth+1); + } + // Check for valid comparator methods + else if canResolveMethod(comparator, "key", data) { + // TODO: check for keyComparator + // Check return type of key + const keydata = comparator.key(data); + type keytype = keydata.type; + if !(canResolve("<", keydata, keydata)) then + compilerError(errorDepth=errorDepth, "The key method in ", comparator.type:string, " must return an object that supports the '<' function when used with ", eltType:string, " elements"); + + // Check that there isn't also a compare or keyPart + if canResolveMethod(comparator, "compare", data, data) { + compilerError(errorDepth=errorDepth, comparator.type:string, " contains both a key method and a compare method"); + } + if canResolveMethod(comparator, "keyPart", data, 0) { + compilerError(errorDepth=errorDepth, comparator.type:string, " contains both a key method and a keyPart method"); + } + } + else if canResolveMethod(comparator, "compare", data, data) { + if doDeprecationCheck { + if !comparatorImplementsRelative(comparator) && + !comparatorImplementsKeyPart(comparator) { + // if there is a keyPart method, we should use that interface + param hasKeyPart = canResolveMethod(comparator, "keyPart", data, 0); + param atType = if isRecord(comparator) then "record" else "class"; + param fixString = "'" + atType + " " + + comparator.type:string + ": relativeComparator'"; + if !hasKeyPart { + compilerWarning(errorDepth=errorDepth, + "Defining a comparator with a 'compare' method without " + + "implementing the relativeComparator interface is deprecated. " + + "Please implement the relativeComparator interface (i.e. " + fixString + ")."); + } else { + compilerWarning(errorDepth=errorDepth, + "Defining a comparator with both a 'compare' method and a 'keyPart' without " + + "implementing the keyPartComparator interface is deprecated. " + + "Please implement the keyPartComparator interface (i.e. " + fixString + ")."); + } + } + } + // Check return type of compare + type comparetype = comparator.compare(data, data).type; + if !(isNumericType(comparetype)) then + compilerError(errorDepth=errorDepth, "The compare method in ", comparator.type:string, " must return a numeric type when used with ", eltType:string, " elements"); + + // if the user has implemented the keyPart interface, we also have to check + // that the keyPart method is implemented correctly to satisfy the interface + if comparatorImplementsKeyPart(comparator) then + chpl_check_comparator_keyPart(comparator, eltType, errorDepth+1, false); + } else { + chpl_check_comparator_keyPart(comparator, eltType, errorDepth+1, doDeprecationCheck); + // TODO: this error should talk about interfaces // If we make it this far, the passed comparator was defined incorrectly compilerError(errorDepth=errorDepth, "The comparator " + comparator.type:string + " requires a 'key(a)', 'compare(a, b)', or 'keyPart(a, i)' method " + " for element type " + eltType:string ); @@ -3402,11 +3448,24 @@ private proc comparatorImplementsKeyPart(cmp) param do @chpldoc.nodoc config param useKeyPartStatus = false; +// TODO: chpldoc will not render this yet :( +// TODO: this is a hack to workaround issues with interfaces +interface relativeComparator { + /* + // return type must be signed integral + proc Self.compare(x, y: x.type); + */ +} +private proc comparatorImplementsRelative(cmp) param do + return __primitive("implements interface", cmp, relativeComparator) != 2; + // TODO: this represents the mutually exclusive OR of keyComparator, // keyPartComparator, and relativeComparator // This cannot be represented in Chapel today, but we still want to // reserve the identifier. // See https://github.com/chapel-lang/chapel/issues/25554. +// TODO: this should be unstable, but can'y apply attributes to interfaces +// @unstable("This is a placeholder for a future feature") interface sortComparator { } /* Default comparator used in sort functions.*/ @@ -3439,6 +3498,7 @@ record DefaultComparator: keyPartComparator { */ inline proc compare(x, y: x.type) { + writeln("foooo"); if x < y { return -1; } else if y < x { return 1; } else return 0; From 65bb469c0a41b152b123b8a34cd917a9c6b700a4 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Tue, 27 Aug 2024 10:45:52 -0700 Subject: [PATCH 02/22] add tests of various error messages with the new interface Signed-off-by: Jade Abraham --- test/deprecated/Sort/compareAndKeyPart.chpl | 32 +++++++++++ .../Sort/compareAndKeyPart.compopts | 3 + .../Sort/compareAndKeyPart.deprecation.good | 4 ++ .../Sort/compareAndKeyPart.error.good | 1 + test/deprecated/Sort/compareAndKeyPart.good | 2 + .../Sort/useRelativeComparator-resolved.good | 6 ++ .../Sort/useRelativeComparator.compopts | 4 ++ .../Sort/useRelativeComparator.good | 12 ++++ .../Sort/useRelativeComparator.prediff | 3 + test/deprecated/Sort/useRelativeCompator.chpl | 57 +++++++++++++++++++ .../errors/error-multiple-interfaces.1.good | 1 + .../errors/error-multiple-interfaces.2.good | 1 + .../errors/error-multiple-interfaces.3.good | 1 + .../errors/error-multiple-interfaces.4.good | 1 + .../errors/error-multiple-interfaces.chpl | 16 ++++++ .../errors/error-multiple-interfaces.compopts | 4 ++ .../standard/Sort/errors/errors-compare.chpl | 9 +++ 17 files changed, 157 insertions(+) create mode 100644 test/deprecated/Sort/compareAndKeyPart.chpl create mode 100644 test/deprecated/Sort/compareAndKeyPart.compopts create mode 100644 test/deprecated/Sort/compareAndKeyPart.deprecation.good create mode 100644 test/deprecated/Sort/compareAndKeyPart.error.good create mode 100644 test/deprecated/Sort/compareAndKeyPart.good create mode 100644 test/deprecated/Sort/useRelativeComparator-resolved.good create mode 100644 test/deprecated/Sort/useRelativeComparator.compopts create mode 100644 test/deprecated/Sort/useRelativeComparator.good create mode 100755 test/deprecated/Sort/useRelativeComparator.prediff create mode 100644 test/deprecated/Sort/useRelativeCompator.chpl create mode 100644 test/library/standard/Sort/errors/error-multiple-interfaces.1.good create mode 100644 test/library/standard/Sort/errors/error-multiple-interfaces.2.good create mode 100644 test/library/standard/Sort/errors/error-multiple-interfaces.3.good create mode 100644 test/library/standard/Sort/errors/error-multiple-interfaces.4.good create mode 100644 test/library/standard/Sort/errors/error-multiple-interfaces.chpl create mode 100644 test/library/standard/Sort/errors/error-multiple-interfaces.compopts diff --git a/test/deprecated/Sort/compareAndKeyPart.chpl b/test/deprecated/Sort/compareAndKeyPart.chpl new file mode 100644 index 000000000000..1691800c59d7 --- /dev/null +++ b/test/deprecated/Sort/compareAndKeyPart.chpl @@ -0,0 +1,32 @@ +use Sort; + +// deprecation, missing keyPartComparator +record R1 { + proc compare(x,y) do return if x 0 then -1 else 0, elt); +} + +// this is an error, keyPart is wrong +record R2: keyPartComparator { + proc compare(x,y) do return if x 0 then -1 else 0, elt); +} + +// this is correct +record R3: keyPartComparator { + proc compare(x,y) do return if x 0 then keyPartStatus.pre else keyPartStatus.returned, elt); +} + + +config type comparator; + +use Random; +var arr: [1..1000] int; + +fillRandom(arr); +writeln("isSorted before ", isSorted(arr, comparator=new comparator())); +// this line won't get the deprecation warning (already resolved in isSorted) +sort(arr, comparator=new comparator()); +writeln("isSorted after ", isSorted(arr, comparator=new comparator())); diff --git a/test/deprecated/Sort/compareAndKeyPart.compopts b/test/deprecated/Sort/compareAndKeyPart.compopts new file mode 100644 index 000000000000..566b449bfd64 --- /dev/null +++ b/test/deprecated/Sort/compareAndKeyPart.compopts @@ -0,0 +1,3 @@ +-scomparator=R1 # compareAndKeyPart.deprecation.good +-scomparator=R2 # compareAndKeyPart.error.good +-scomparator=R3 # compareAndKeyPart.good diff --git a/test/deprecated/Sort/compareAndKeyPart.deprecation.good b/test/deprecated/Sort/compareAndKeyPart.deprecation.good new file mode 100644 index 000000000000..a46abdc279bf --- /dev/null +++ b/test/deprecated/Sort/compareAndKeyPart.deprecation.good @@ -0,0 +1,4 @@ +compareAndKeyPart.chpl:29: warning: Defining a comparator with both a 'compare' method and a 'keyPart' without implementing the keyPartComparator interface is deprecated. Please implement the keyPartComparator interface (i.e. 'record R1: relativeComparator'). +compareAndKeyPart.chpl:32: warning: Defining a comparator with both a 'compare' method and a 'keyPart' without implementing the keyPartComparator interface is deprecated. Please implement the keyPartComparator interface (i.e. 'record R1: relativeComparator'). +isSorted before false +isSorted after true diff --git a/test/deprecated/Sort/compareAndKeyPart.error.good b/test/deprecated/Sort/compareAndKeyPart.error.good new file mode 100644 index 000000000000..fbf2e4f0739c --- /dev/null +++ b/test/deprecated/Sort/compareAndKeyPart.error.good @@ -0,0 +1 @@ +compareAndKeyPart.chpl:29: error: The keyPart method in R2 must return a tuple with element 0 of type keyPartStatus when used with int(64) elements diff --git a/test/deprecated/Sort/compareAndKeyPart.good b/test/deprecated/Sort/compareAndKeyPart.good new file mode 100644 index 000000000000..c30307016d46 --- /dev/null +++ b/test/deprecated/Sort/compareAndKeyPart.good @@ -0,0 +1,2 @@ +isSorted before false +isSorted after true diff --git a/test/deprecated/Sort/useRelativeComparator-resolved.good b/test/deprecated/Sort/useRelativeComparator-resolved.good new file mode 100644 index 000000000000..560a1534a442 --- /dev/null +++ b/test/deprecated/Sort/useRelativeComparator-resolved.good @@ -0,0 +1,6 @@ +isSorted before false +isSorted after true +isSorted before false +isSorted after true +isSorted before false +isSorted after true diff --git a/test/deprecated/Sort/useRelativeComparator.compopts b/test/deprecated/Sort/useRelativeComparator.compopts new file mode 100644 index 000000000000..7ae273a714fa --- /dev/null +++ b/test/deprecated/Sort/useRelativeComparator.compopts @@ -0,0 +1,4 @@ + # useRelativeComparator.good +-sresolveDeps=false # useRelativeComparator.good +-sresolveDeps # useRelativeComparator-resolved.good +-sresolveDeps=true # useRelativeComparator-resolved.good diff --git a/test/deprecated/Sort/useRelativeComparator.good b/test/deprecated/Sort/useRelativeComparator.good new file mode 100644 index 000000000000..62cb24a2ef7d --- /dev/null +++ b/test/deprecated/Sort/useRelativeComparator.good @@ -0,0 +1,12 @@ +useRelativeCompator.chpl:44: warning: Defining a comparator with a 'compare' method without implementing the relativeComparator interface is deprecated. Please implement the relativeComparator interface (i.e. 'record myCmp1: relativeComparator'). +useRelativeCompator.chpl:45: warning: Defining a comparator with a 'compare' method without implementing the relativeComparator interface is deprecated. Please implement the relativeComparator interface (i.e. 'record myCmp2: relativeComparator'). +useRelativeCompator.chpl:46: warning: Defining a comparator with a 'compare' method without implementing the relativeComparator interface is deprecated. Please implement the relativeComparator interface (i.e. 'record myCmp3: relativeComparator'). +useRelativeCompator.chpl:49: warning: Defining a comparator with a 'compare' method without implementing the relativeComparator interface is deprecated. Please implement the relativeComparator interface (i.e. 'record myCmp4: relativeComparator'). +useRelativeCompator.chpl:50: warning: Defining a comparator with a 'compare' method without implementing the relativeComparator interface is deprecated. Please implement the relativeComparator interface (i.e. 'record myCmp5: relativeComparator'). +useRelativeCompator.chpl:51: warning: Defining a comparator with a 'compare' method without implementing the relativeComparator interface is deprecated. Please implement the relativeComparator interface (i.e. 'record myCmp6: relativeComparator'). +useRelativeCompator.chpl:54: warning: Defining a comparator with a 'compare' method without implementing the relativeComparator interface is deprecated. Please implement the relativeComparator interface (i.e. 'record myCmp7: relativeComparator'). +$CHPL_HOME/modules/standard/Sort.chpl:nnnn: In iterator 'sorted': +$CHPL_HOME/modules/standard/Sort.chpl:nnnn: warning: Defining a comparator with a 'compare' method without implementing the relativeComparator interface is deprecated. Please implement the relativeComparator interface (i.e. 'record myCmp8: relativeComparator'). + useRelativeCompator.chpl:55: called as sorted(x: [domain(1,int(64),one)] int(64), comparator: myCmp8) +note: generic instantiations are underlined in the above callstack +useRelativeCompator.chpl:56: note: Defining a comparator with a 'compare' method without implementing the relativeComparator interface is deprecated. Please implement the relativeComparator interface (i.e. 'record myCmp9: relativeComparator'). diff --git a/test/deprecated/Sort/useRelativeComparator.prediff b/test/deprecated/Sort/useRelativeComparator.prediff new file mode 100755 index 000000000000..523a8612ae47 --- /dev/null +++ b/test/deprecated/Sort/useRelativeComparator.prediff @@ -0,0 +1,3 @@ +#!/usr/bin/env bash + +$CHPL_HOME/util/test/prediff-obscure-module-linenos $@ diff --git a/test/deprecated/Sort/useRelativeCompator.chpl b/test/deprecated/Sort/useRelativeCompator.chpl new file mode 100644 index 000000000000..3224cd7b906d --- /dev/null +++ b/test/deprecated/Sort/useRelativeCompator.chpl @@ -0,0 +1,57 @@ +use Sort; + +config param resolveDeps = false; + +// This is a simple comparator for types with '<' +// It is identical to the Sort module for normal sorting +// To make sure we get all the deprecation warnings, we need to use a unique comparator type for each call + +record myCmp1{proc compare(x,y) do return if x Date: Tue, 27 Aug 2024 10:53:02 -0700 Subject: [PATCH 03/22] fix typo Signed-off-by: Jade Abraham --- modules/standard/Sort.chpl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/standard/Sort.chpl b/modules/standard/Sort.chpl index ef11fed5a1a8..cdcdc1643721 100644 --- a/modules/standard/Sort.chpl +++ b/modules/standard/Sort.chpl @@ -3464,7 +3464,7 @@ private proc comparatorImplementsRelative(cmp) param do // This cannot be represented in Chapel today, but we still want to // reserve the identifier. // See https://github.com/chapel-lang/chapel/issues/25554. -// TODO: this should be unstable, but can'y apply attributes to interfaces +// TODO: this should be unstable, but can't apply attributes to interfaces // @unstable("This is a placeholder for a future feature") interface sortComparator { } From 371b607c006d6b4af6073076d32a590eddf627e3 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Tue, 27 Aug 2024 11:16:05 -0700 Subject: [PATCH 04/22] fix spacing Signed-off-by: Jade Abraham --- modules/standard/Sort.chpl | 2 +- .../standard/Sort/errors/error-multiple-interfaces.2.good | 2 +- .../standard/Sort/errors/error-multiple-interfaces.3.good | 2 +- .../standard/Sort/errors/errors-comparator.compareargs.good | 2 +- test/library/standard/Sort/errors/errors-comparator.empty.good | 2 +- .../library/standard/Sort/errors/errors-comparator.keyargs.good | 2 +- .../standard/Sort/errors/errors-comparator.keypartargs.good | 2 +- test/library/standard/Sort/errors/errors-comparator.wrong.good | 2 +- 8 files changed, 8 insertions(+), 8 deletions(-) diff --git a/modules/standard/Sort.chpl b/modules/standard/Sort.chpl index cdcdc1643721..29e1ab680534 100644 --- a/modules/standard/Sort.chpl +++ b/modules/standard/Sort.chpl @@ -499,7 +499,7 @@ proc chpl_check_comparator(comparator, // TODO: this error should talk about interfaces // If we make it this far, the passed comparator was defined incorrectly - compilerError(errorDepth=errorDepth, "The comparator " + comparator.type:string + " requires a 'key(a)', 'compare(a, b)', or 'keyPart(a, i)' method " + " for element type " + eltType:string ); + compilerError(errorDepth=errorDepth, "The comparator " + comparator.type:string + " requires a 'key(a)', 'compare(a, b)', or 'keyPart(a, i)' method for element type " + eltType:string ); } return true; diff --git a/test/library/standard/Sort/errors/error-multiple-interfaces.2.good b/test/library/standard/Sort/errors/error-multiple-interfaces.2.good index bf12c081f518..58c64e870b39 100644 --- a/test/library/standard/Sort/errors/error-multiple-interfaces.2.good +++ b/test/library/standard/Sort/errors/error-multiple-interfaces.2.good @@ -1 +1 @@ -error-multiple-interfaces.chpl:16: error: The comparator R2 requires a 'key(a)', 'compare(a, b)', or 'keyPart(a, i)' method for element type int(64) +error-multiple-interfaces.chpl:16: error: The comparator R2 requires a 'key(a)', 'compare(a, b)', or 'keyPart(a, i)' method for element type int(64) diff --git a/test/library/standard/Sort/errors/error-multiple-interfaces.3.good b/test/library/standard/Sort/errors/error-multiple-interfaces.3.good index 440120633d53..8c98034234f1 100644 --- a/test/library/standard/Sort/errors/error-multiple-interfaces.3.good +++ b/test/library/standard/Sort/errors/error-multiple-interfaces.3.good @@ -1 +1 @@ -error-multiple-interfaces.chpl:16: error: The comparator R3 requires a 'key(a)', 'compare(a, b)', or 'keyPart(a, i)' method for element type int(64) +error-multiple-interfaces.chpl:16: error: The comparator R3 requires a 'key(a)', 'compare(a, b)', or 'keyPart(a, i)' method for element type int(64) diff --git a/test/library/standard/Sort/errors/errors-comparator.compareargs.good b/test/library/standard/Sort/errors/errors-comparator.compareargs.good index 473dd0eb8b2a..ff62b14fe4c2 100644 --- a/test/library/standard/Sort/errors/errors-comparator.compareargs.good +++ b/test/library/standard/Sort/errors/errors-comparator.compareargs.good @@ -1,2 +1,2 @@ errors-comparator.chpl:9: In function 'main': -errors-comparator.chpl:17: error: The comparator compareargs requires a 'key(a)', 'compare(a, b)', or 'keyPart(a, i)' method for element type int(64) +errors-comparator.chpl:17: error: The comparator compareargs requires a 'key(a)', 'compare(a, b)', or 'keyPart(a, i)' method for element type int(64) diff --git a/test/library/standard/Sort/errors/errors-comparator.empty.good b/test/library/standard/Sort/errors/errors-comparator.empty.good index fce595fe0771..d5ae18b99ce1 100644 --- a/test/library/standard/Sort/errors/errors-comparator.empty.good +++ b/test/library/standard/Sort/errors/errors-comparator.empty.good @@ -1,2 +1,2 @@ errors-comparator.chpl:9: In function 'main': -errors-comparator.chpl:17: error: The comparator empty requires a 'key(a)', 'compare(a, b)', or 'keyPart(a, i)' method for element type int(64) +errors-comparator.chpl:17: error: The comparator empty requires a 'key(a)', 'compare(a, b)', or 'keyPart(a, i)' method for element type int(64) diff --git a/test/library/standard/Sort/errors/errors-comparator.keyargs.good b/test/library/standard/Sort/errors/errors-comparator.keyargs.good index b3785405b8f8..c087d315a212 100644 --- a/test/library/standard/Sort/errors/errors-comparator.keyargs.good +++ b/test/library/standard/Sort/errors/errors-comparator.keyargs.good @@ -1,2 +1,2 @@ errors-comparator.chpl:9: In function 'main': -errors-comparator.chpl:17: error: The comparator keyargs requires a 'key(a)', 'compare(a, b)', or 'keyPart(a, i)' method for element type int(64) +errors-comparator.chpl:17: error: The comparator keyargs requires a 'key(a)', 'compare(a, b)', or 'keyPart(a, i)' method for element type int(64) diff --git a/test/library/standard/Sort/errors/errors-comparator.keypartargs.good b/test/library/standard/Sort/errors/errors-comparator.keypartargs.good index 79869186d469..2978bdfe8bb3 100644 --- a/test/library/standard/Sort/errors/errors-comparator.keypartargs.good +++ b/test/library/standard/Sort/errors/errors-comparator.keypartargs.good @@ -1,2 +1,2 @@ errors-comparator.chpl:9: In function 'main': -errors-comparator.chpl:17: error: The comparator keyPartArgs requires a 'key(a)', 'compare(a, b)', or 'keyPart(a, i)' method for element type int(64) +errors-comparator.chpl:17: error: The comparator keyPartArgs requires a 'key(a)', 'compare(a, b)', or 'keyPart(a, i)' method for element type int(64) diff --git a/test/library/standard/Sort/errors/errors-comparator.wrong.good b/test/library/standard/Sort/errors/errors-comparator.wrong.good index bd93885b5047..3004281de049 100644 --- a/test/library/standard/Sort/errors/errors-comparator.wrong.good +++ b/test/library/standard/Sort/errors/errors-comparator.wrong.good @@ -1,2 +1,2 @@ errors-comparator.chpl:9: In function 'main': -errors-comparator.chpl:17: error: The comparator wrong requires a 'key(a)', 'compare(a, b)', or 'keyPart(a, i)' method for element type int(64) +errors-comparator.chpl:17: error: The comparator wrong requires a 'key(a)', 'compare(a, b)', or 'keyPart(a, i)' method for element type int(64) From 7b8d1cb973431ebe49df1213f762475334bf3134 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Tue, 27 Aug 2024 11:19:58 -0700 Subject: [PATCH 05/22] fix error handling Signed-off-by: Jade Abraham --- modules/standard/Sort.chpl | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/modules/standard/Sort.chpl b/modules/standard/Sort.chpl index 29e1ab680534..78db3eaa11be 100644 --- a/modules/standard/Sort.chpl +++ b/modules/standard/Sort.chpl @@ -413,6 +413,8 @@ proc chpl_check_comparator_keyPart(comparator, compilerError(errorDepth=errorDepth, "The keyPart method in ", comparator.type:string, " must return a tuple with element 1 of type int(?) or uint(?) when used with ", eltType:string, " elements"); } } + + return true; } pragma "unsafe" // due to 'data' default-initialized to nil for class types @@ -493,9 +495,10 @@ proc chpl_check_comparator(comparator, // that the keyPart method is implemented correctly to satisfy the interface if comparatorImplementsKeyPart(comparator) then chpl_check_comparator_keyPart(comparator, eltType, errorDepth+1, false); + } else if chpl_check_comparator_keyPart(comparator, eltType, errorDepth+1, doDeprecationCheck) { + // the check and error are in chpl_check_comparator_keyPart } else { - chpl_check_comparator_keyPart(comparator, eltType, errorDepth+1, doDeprecationCheck); // TODO: this error should talk about interfaces // If we make it this far, the passed comparator was defined incorrectly From 41876707442e9f0cd76aed9d7c0eddf22f098094 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Tue, 27 Aug 2024 13:10:01 -0700 Subject: [PATCH 06/22] add unstable Signed-off-by: Jade Abraham --- modules/standard/Sort.chpl | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/modules/standard/Sort.chpl b/modules/standard/Sort.chpl index 78db3eaa11be..b5f164490e70 100644 --- a/modules/standard/Sort.chpl +++ b/modules/standard/Sort.chpl @@ -3433,6 +3433,7 @@ private inline proc reverseKeyPartStatus(status) do // TODO: chpldoc will not render this yet :( // TODO: this is a hack to workaround issues with interfaces +// TODO: @unstable("keyPartComparator is not yet stable") interface keyPartComparator { /* proc Self.keyPart(elt, i: int): (keyPartStatus, integral); @@ -3453,6 +3454,7 @@ config param useKeyPartStatus = false; // TODO: chpldoc will not render this yet :( // TODO: this is a hack to workaround issues with interfaces +// TODO: @unstable("relativeComparator is not yet stable") interface relativeComparator { /* // return type must be signed integral @@ -3468,7 +3470,7 @@ private proc comparatorImplementsRelative(cmp) param do // reserve the identifier. // See https://github.com/chapel-lang/chapel/issues/25554. // TODO: this should be unstable, but can't apply attributes to interfaces -// @unstable("This is a placeholder for a future feature") +// @unstable("sortComparator is not yet stable") interface sortComparator { } /* Default comparator used in sort functions.*/ From 30bb52f92a05b8ab3aba6a9df99f8fff276fb58a Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Tue, 27 Aug 2024 13:27:52 -0700 Subject: [PATCH 07/22] remove debug print Signed-off-by: Jade Abraham --- modules/standard/Sort.chpl | 1 - 1 file changed, 1 deletion(-) diff --git a/modules/standard/Sort.chpl b/modules/standard/Sort.chpl index b5f164490e70..45c492faa1ce 100644 --- a/modules/standard/Sort.chpl +++ b/modules/standard/Sort.chpl @@ -3503,7 +3503,6 @@ record DefaultComparator: keyPartComparator { */ inline proc compare(x, y: x.type) { - writeln("foooo"); if x < y { return -1; } else if y < x { return 1; } else return 0; From 9e0300172710de475eea21ca47b2da4fd795b916 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Tue, 27 Aug 2024 13:28:01 -0700 Subject: [PATCH 08/22] text test name Signed-off-by: Jade Abraham --- .../Sort/{useRelativeCompator.chpl => useRelativeComparator.chpl} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename test/deprecated/Sort/{useRelativeCompator.chpl => useRelativeComparator.chpl} (100%) diff --git a/test/deprecated/Sort/useRelativeCompator.chpl b/test/deprecated/Sort/useRelativeComparator.chpl similarity index 100% rename from test/deprecated/Sort/useRelativeCompator.chpl rename to test/deprecated/Sort/useRelativeComparator.chpl From fe1b035d5b21d7f88fc35e95ccde641e2b9d0933 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Tue, 27 Aug 2024 13:29:10 -0700 Subject: [PATCH 09/22] update tests Signed-off-by: Jade Abraham --- .../Sort/correctness/correctness.chpl | 4 +- .../Sort/correctness/sort-array-of-owned.chpl | 42 +++++++++++++------ .../Sort/correctness/sort-region.chpl | 2 +- 3 files changed, 32 insertions(+), 16 deletions(-) diff --git a/test/library/standard/Sort/correctness/correctness.chpl b/test/library/standard/Sort/correctness/correctness.chpl index 442339452c25..8a8470e5153d 100644 --- a/test/library/standard/Sort/correctness/correctness.chpl +++ b/test/library/standard/Sort/correctness/correctness.chpl @@ -237,11 +237,11 @@ class AbsKeyCmpClass { /* Compare Sort by absolute value */ -record AbsCompCmp { +record AbsCompCmp: relativeComparator { proc compare(a, b) { return abs(a) - abs(b); } proc name() { return 'AbsCompCmp'; } } -class AbsCompCmpClass { +class AbsCompCmpClass: relativeComparator { proc compare(a, b) { return abs(a) - abs(b); } proc name() { return 'AbsCompCmpClass'; } } diff --git a/test/library/standard/Sort/correctness/sort-array-of-owned.chpl b/test/library/standard/Sort/correctness/sort-array-of-owned.chpl index ae3da4db64f3..1a33b5483c05 100644 --- a/test/library/standard/Sort/correctness/sort-array-of-owned.chpl +++ b/test/library/standard/Sort/correctness/sort-array-of-owned.chpl @@ -8,14 +8,16 @@ class Value { } -record ValueComparator { - proc key(v: Value?) where useKeyComparator { +record keyValueComparator { + proc key(v: Value?) { if v == nil then return 0; else return v!.x; } - proc compare(a: Value?, b: Value?) where !useKeyComparator { +} +record compareValueComparator: relativeComparator { + proc compare(a: Value?, b: Value?) { var aNum = 0; if a != nil then aNum = a!.x; @@ -25,7 +27,8 @@ record ValueComparator { return aNum - bNum; } } - +proc ValueComparator type do + return if useKeyComparator then keyValueComparator else compareValueComparator; // TEST 1: SORTING OWNED ELEMENTS { @@ -55,14 +58,16 @@ operator Wrapper.=(ref lhs: Wrapper, ref rhs: Wrapper) { lhs.elt = rhs.elt; } -record WrapperComparator { - proc key(v: Wrapper) where useKeyComparator { +record keyWrapperComparator { + proc key(v: Wrapper) { if v.elt == nil then return 0; else return v.elt!.x; } - proc compare(a: Wrapper, b: Wrapper) where !useKeyComparator { +} +record compareWrapperComparator: relativeComparator { + proc compare(a: Wrapper, b: Wrapper) { var aNum = 0; if a.elt != nil then aNum = a.elt!.x; @@ -73,6 +78,8 @@ record WrapperComparator { } } +proc WrapperComparator type do + return if useKeyComparator then keyWrapperComparator else compareWrapperComparator; { writeln("Testing sorting record containing owned"); @@ -101,14 +108,16 @@ record ArrayWrapper { elts[1] = arg; } } -record ArrayWrapperComparator { - proc key(v: ArrayWrapper) where useKeyComparator { +record keyArrayWrapperComparator { + proc key(v: ArrayWrapper) { if v.elts[1] == nil then return 0; else return v.elts[1]!.x; } - proc compare(a: ArrayWrapper, b: ArrayWrapper) where !useKeyComparator { +} +record compareArrayWrapperComparator: relativeComparator { + proc compare(a: ArrayWrapper, b: ArrayWrapper) { var aNum = 0; if a.elts[1] != nil then aNum = a.elts[1]!.x; @@ -118,6 +127,9 @@ record ArrayWrapperComparator { return aNum - bNum; } } +proc ArrayWrapperComparator type do + return if useKeyComparator then keyArrayWrapperComparator + else compareArrayWrapperComparator; { writeln("Testing sorting record containing array of owned"); @@ -136,14 +148,16 @@ record ArrayWrapperComparator { // TEST 4: SORTING ARRAY CONTAINING ARRAY OF OWNED ELEMENTS -record ArrayComparator { - proc key(v: [] owned Value?) where useKeyComparator { +record keyArrayComparator { + proc key(v: [] owned Value?) { if v[1] == nil then return 0; else return v[1]!.x; } - proc compare(a: [] owned Value?, b: [] owned Value?) where !useKeyComparator { +} +record compareArrayComparator: relativeComparator { + proc compare(a: [] owned Value?, b: [] owned Value?) { var aNum = 0; if a[1] != nil then aNum = a[1]!.x; @@ -153,6 +167,8 @@ record ArrayComparator { return aNum - bNum; } } +proc ArrayComparator type do + return if useKeyComparator then keyArrayComparator else compareArrayComparator; { writeln("Testing sorting array containing array of owned"); diff --git a/test/library/standard/Sort/correctness/sort-region.chpl b/test/library/standard/Sort/correctness/sort-region.chpl index 639d0de554b0..c4b75a614288 100644 --- a/test/library/standard/Sort/correctness/sort-region.chpl +++ b/test/library/standard/Sort/correctness/sort-region.chpl @@ -1,6 +1,6 @@ use Sort; -record AbsCompCmp { +record AbsCompCmp: relativeComparator { proc compare(a, b) { return abs(a) - abs(b); } proc name() { return 'AbsCompCmp'; } } From 5e9fdd2409dd686f3e144090047c423e31c9831a Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Tue, 27 Aug 2024 15:04:27 -0700 Subject: [PATCH 10/22] add todo for later Signed-off-by: Jade Abraham --- modules/standard/Sort.chpl | 2 ++ 1 file changed, 2 insertions(+) diff --git a/modules/standard/Sort.chpl b/modules/standard/Sort.chpl index 45c492faa1ce..61d326f38184 100644 --- a/modules/standard/Sort.chpl +++ b/modules/standard/Sort.chpl @@ -332,6 +332,8 @@ proc compareByPart(a:?t, b:t, comparator:?rec) { a == b: returns 0 */ inline proc chpl_compare(a:?t, b:t, comparator:?rec) { + // TODO: this should also check interfaces for deprecation because it gets + // used in things like Search, Heap, treap, SortedSet, SortedMap, etc. // TODO -- In cases where values are larger than keys, it may be faster to // key data once and sort the keyed data, mirroring swaps in data. // Compare results of comparator.key(a) if is defined by user From e5ed4e695c6d3aca66fafbd6d54e7c96b78833b1 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Tue, 27 Aug 2024 15:04:45 -0700 Subject: [PATCH 11/22] fix more uses of the old comparators Signed-off-by: Jade Abraham --- test/deprecated/Sort/useRelativeComparator.good | 6 ++++++ test/exercises/duplicates/duplicates-bonus-b2-record.chpl | 2 +- test/exercises/duplicates/duplicates-bonus-b3-size.chpl | 2 +- .../draft/Vector/sort/vectorSortOtherComparator.chpl | 1 + test/library/packages/Search/correctness.chpl | 6 +++--- .../library/standard/List/sort/listSortOtherComparator.chpl | 1 + .../comd/elegant/arrayOfStructs/util/Simulation.chpl | 2 +- tools/mason/MasonSearch.chpl | 2 +- tools/unstableWarningAnonymizer/unstableAnonScript.chpl | 1 + 9 files changed, 16 insertions(+), 7 deletions(-) diff --git a/test/deprecated/Sort/useRelativeComparator.good b/test/deprecated/Sort/useRelativeComparator.good index 62cb24a2ef7d..446e8c9fa97f 100644 --- a/test/deprecated/Sort/useRelativeComparator.good +++ b/test/deprecated/Sort/useRelativeComparator.good @@ -10,3 +10,9 @@ $CHPL_HOME/modules/standard/Sort.chpl:nnnn: warning: Defining a comparator with useRelativeCompator.chpl:55: called as sorted(x: [domain(1,int(64),one)] int(64), comparator: myCmp8) note: generic instantiations are underlined in the above callstack useRelativeCompator.chpl:56: note: Defining a comparator with a 'compare' method without implementing the relativeComparator interface is deprecated. Please implement the relativeComparator interface (i.e. 'record myCmp9: relativeComparator'). +isSorted before false +isSorted after true +isSorted before false +isSorted after true +isSorted before false +isSorted after true diff --git a/test/exercises/duplicates/duplicates-bonus-b2-record.chpl b/test/exercises/duplicates/duplicates-bonus-b2-record.chpl index 2f6d0db940f8..a2122b994d49 100644 --- a/test/exercises/duplicates/duplicates-bonus-b2-record.chpl +++ b/test/exercises/duplicates/duplicates-bonus-b2-record.chpl @@ -18,7 +18,7 @@ record HashedPath { // stumbling block: no < for record // stumbling block: comparator increases complexity // stumbling block: need to know SHA256Hash has compare and/or < -record HashedPathComparator { +record HashedPathComparator: relativeComparator { proc compare(a: HashedPath, b: HashedPath) { if a.hash < b.hash { return -1; diff --git a/test/exercises/duplicates/duplicates-bonus-b3-size.chpl b/test/exercises/duplicates/duplicates-bonus-b3-size.chpl index b7b16210a0b1..271c9de60582 100644 --- a/test/exercises/duplicates/duplicates-bonus-b3-size.chpl +++ b/test/exercises/duplicates/duplicates-bonus-b3-size.chpl @@ -19,7 +19,7 @@ record HashedPath { var path: string; } -record HashedPathComparator { +record HashedPathComparator: relativeComparator { proc compare(a: HashedPath, b: HashedPath) { if a.size < b.size { return -1; diff --git a/test/library/draft/Vector/sort/vectorSortOtherComparator.chpl b/test/library/draft/Vector/sort/vectorSortOtherComparator.chpl index d6a3ec024d19..7c5f526b787c 100644 --- a/test/library/draft/Vector/sort/vectorSortOtherComparator.chpl +++ b/test/library/draft/Vector/sort/vectorSortOtherComparator.chpl @@ -22,6 +22,7 @@ vec2.clear(); // comparator (this is just absval). // record myComparator {} +myComparator implements relativeComparator; proc myComparator.compare(a, b) { return abs(a) - abs(b); } diff --git a/test/library/packages/Search/correctness.chpl b/test/library/packages/Search/correctness.chpl index e690a373ba8d..354e568048fb 100644 --- a/test/library/packages/Search/correctness.chpl +++ b/test/library/packages/Search/correctness.chpl @@ -124,12 +124,12 @@ record AbsKeyCmp { /* Compare Sort by absolute value */ -record AbsCompCmp { +record AbsCompCmp: relativeComparator { proc compare(a, b) { return abs(a) - abs(b); } proc name() { return 'AbsCompCmp'; } } - +// unused /* Key method should take priority over compare method */ record AbsKeyCompCmp { proc key(a) { return abs(a); } @@ -137,7 +137,7 @@ record AbsKeyCompCmp { proc name() { return 'AbsKeyCompCmp'; } } - +// unused /* Key method can return a non-numerical/string type, such as tuple */ record TupleCmp { proc key(a) { return (a, a); } diff --git a/test/library/standard/List/sort/listSortOtherComparator.chpl b/test/library/standard/List/sort/listSortOtherComparator.chpl index 267ecc6e5547..686b7f3d3a9f 100644 --- a/test/library/standard/List/sort/listSortOtherComparator.chpl +++ b/test/library/standard/List/sort/listSortOtherComparator.chpl @@ -22,6 +22,7 @@ lst2.clear(); // comparator (this is just absval). // record myComparator {} +myComparator implements relativeComparator; proc myComparator.compare(a, b) { return abs(a) - abs(b); } diff --git a/test/studies/comd/elegant/arrayOfStructs/util/Simulation.chpl b/test/studies/comd/elegant/arrayOfStructs/util/Simulation.chpl index 48b9f2e0fa5d..c8c8118c5448 100644 --- a/test/studies/comd/elegant/arrayOfStructs/util/Simulation.chpl +++ b/test/studies/comd/elegant/arrayOfStructs/util/Simulation.chpl @@ -392,7 +392,7 @@ proc pbc() { proc sortAtomsInCell() { use Sort; - record cmp { + record cmp: relativeComparator { proc compare(a, b) { return a.gid - b.gid; } diff --git a/tools/mason/MasonSearch.chpl b/tools/mason/MasonSearch.chpl index 8bed0238d988..83d303a25acf 100644 --- a/tools/mason/MasonSearch.chpl +++ b/tools/mason/MasonSearch.chpl @@ -134,7 +134,7 @@ proc masonSearch(ref args: list(string)) { } } -record RankResultsComparator { +record RankResultsComparator: relativeComparator { var query: string; var packageScores; proc compare(a, b) { diff --git a/tools/unstableWarningAnonymizer/unstableAnonScript.chpl b/tools/unstableWarningAnonymizer/unstableAnonScript.chpl index daad0f362e0f..5654a1f15876 100644 --- a/tools/unstableWarningAnonymizer/unstableAnonScript.chpl +++ b/tools/unstableWarningAnonymizer/unstableAnonScript.chpl @@ -167,6 +167,7 @@ inline proc csvPrintArr(arr: [] (string, int, int), // Comparator to sort our array representation of the map // by the number of occurences of each warning record occurenceComparator {} +occurenceComparator implements relativeComparator proc occurenceComparator.compare(a: (string, int, int), b: (string, int, int)){ return b[1] - a[1]; // Reverse sort } From 9e98dd9e07fd0bc27c43effbec95ceeb89575b63 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Tue, 27 Aug 2024 15:04:57 -0700 Subject: [PATCH 12/22] fix channel Signed-off-by: Jade Abraham --- modules/packages/Channel.chpl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/packages/Channel.chpl b/modules/packages/Channel.chpl index 35c6d62800fa..74714f10ac05 100644 --- a/modules/packages/Channel.chpl +++ b/modules/packages/Channel.chpl @@ -527,7 +527,7 @@ module Channel { addresses. */ @chpldoc.nodoc - record Comparator { + record Comparator: relativeComparator { proc compare(case1, case2) { return (case1.getAddr() - case2.getAddr()) : int; } From 358d20f8a11e4d4d8fd5cec74acd11811148f61b Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Tue, 27 Aug 2024 17:20:52 -0700 Subject: [PATCH 13/22] fix error messages Signed-off-by: Jade Abraham --- modules/standard/Sort.chpl | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/modules/standard/Sort.chpl b/modules/standard/Sort.chpl index 61d326f38184..6bee5f5cc224 100644 --- a/modules/standard/Sort.chpl +++ b/modules/standard/Sort.chpl @@ -377,6 +377,8 @@ proc chpl_check_comparator_keyPart(comparator, if !(isInt(expectIntUint) || isUint(expectIntUint)) then compilerError(errorDepth=errorDepth, "The keyPart method in ", comparator.type:string, " must return a tuple with element 1 of type int(?) or uint(?) when used with ", eltType:string, " elements"); // no need to check the interface,'chpl_keyPartInternal' is internal + + return true; } else if canResolveMethod(comparator, "keyPart", data, 0) { if comparatorImplementsKeyPart(comparator) { @@ -414,9 +416,9 @@ proc chpl_check_comparator_keyPart(comparator, if !(isInt(expectIntUint) || isUint(expectIntUint)) then compilerError(errorDepth=errorDepth, "The keyPart method in ", comparator.type:string, " must return a tuple with element 1 of type int(?) or uint(?) when used with ", eltType:string, " elements"); } + return true; } - - return true; + return false; } pragma "unsafe" // due to 'data' default-initialized to nil for class types @@ -496,7 +498,11 @@ proc chpl_check_comparator(comparator, // if the user has implemented the keyPart interface, we also have to check // that the keyPart method is implemented correctly to satisfy the interface if comparatorImplementsKeyPart(comparator) then - chpl_check_comparator_keyPart(comparator, eltType, errorDepth+1, false); + if !chpl_check_comparator_keyPart(comparator, + eltType, + errorDepth+1, + doDeprecationCheck=false) then + compilerError(errorDepth=errorDepth, "The comparator " + comparator.type:string + " implements the keyPartComparator interface, but the keyPart method is not implemented"); } else if chpl_check_comparator_keyPart(comparator, eltType, errorDepth+1, doDeprecationCheck) { // the check and error are in chpl_check_comparator_keyPart } From a7824286abd399b2d8313596fe364486ed078282 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Tue, 27 Aug 2024 17:21:01 -0700 Subject: [PATCH 14/22] fix more tests Signed-off-by: Jade Abraham --- test/library/packages/Search/correctness.chpl | 2 +- .../errors/error-multiple-interfaces.1.good | 2 +- .../errors/error-multiple-interfaces.2-0.good | 0 .../errors/error-multiple-interfaces.2.good | 1 - .../errors/error-multiple-interfaces.3-0.good | 0 .../errors/error-multiple-interfaces.3.good | 1 - .../errors/error-multiple-interfaces.4.good | 2 +- .../errors/error-multiple-interfaces.chpl | 24 +++++++++++++++---- .../errors/error-multiple-interfaces.compopts | 2 +- .../Sort/errors/errors-compare.compopts | 1 + .../Sort/errors/errors-compare.noKeyPart.good | 2 ++ 11 files changed, 27 insertions(+), 10 deletions(-) create mode 100644 test/library/standard/Sort/errors/error-multiple-interfaces.2-0.good delete mode 100644 test/library/standard/Sort/errors/error-multiple-interfaces.2.good create mode 100644 test/library/standard/Sort/errors/error-multiple-interfaces.3-0.good delete mode 100644 test/library/standard/Sort/errors/error-multiple-interfaces.3.good create mode 100644 test/library/standard/Sort/errors/errors-compare.noKeyPart.good diff --git a/test/library/packages/Search/correctness.chpl b/test/library/packages/Search/correctness.chpl index 354e568048fb..44e43641c291 100644 --- a/test/library/packages/Search/correctness.chpl +++ b/test/library/packages/Search/correctness.chpl @@ -2,7 +2,7 @@ * Check correctness of search functions */ -use Search; +use Search; use Sort only relativeComparator; proc main() { diff --git a/test/library/standard/Sort/errors/error-multiple-interfaces.1.good b/test/library/standard/Sort/errors/error-multiple-interfaces.1.good index 4c0e7ca37560..d6892a150a3c 100644 --- a/test/library/standard/Sort/errors/error-multiple-interfaces.1.good +++ b/test/library/standard/Sort/errors/error-multiple-interfaces.1.good @@ -1 +1 @@ -error-multiple-interfaces.chpl:16: error: The comparator R1 should only implement one sort comparator interface. +error-multiple-interfaces.chpl:32: error: The comparator R1 should only implement one sort comparator interface. diff --git a/test/library/standard/Sort/errors/error-multiple-interfaces.2-0.good b/test/library/standard/Sort/errors/error-multiple-interfaces.2-0.good new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/test/library/standard/Sort/errors/error-multiple-interfaces.2.good b/test/library/standard/Sort/errors/error-multiple-interfaces.2.good deleted file mode 100644 index 58c64e870b39..000000000000 --- a/test/library/standard/Sort/errors/error-multiple-interfaces.2.good +++ /dev/null @@ -1 +0,0 @@ -error-multiple-interfaces.chpl:16: error: The comparator R2 requires a 'key(a)', 'compare(a, b)', or 'keyPart(a, i)' method for element type int(64) diff --git a/test/library/standard/Sort/errors/error-multiple-interfaces.3-0.good b/test/library/standard/Sort/errors/error-multiple-interfaces.3-0.good new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/test/library/standard/Sort/errors/error-multiple-interfaces.3.good b/test/library/standard/Sort/errors/error-multiple-interfaces.3.good deleted file mode 100644 index 8c98034234f1..000000000000 --- a/test/library/standard/Sort/errors/error-multiple-interfaces.3.good +++ /dev/null @@ -1 +0,0 @@ -error-multiple-interfaces.chpl:16: error: The comparator R3 requires a 'key(a)', 'compare(a, b)', or 'keyPart(a, i)' method for element type int(64) diff --git a/test/library/standard/Sort/errors/error-multiple-interfaces.4.good b/test/library/standard/Sort/errors/error-multiple-interfaces.4.good index 83eda888904a..c6e798cbaf2a 100644 --- a/test/library/standard/Sort/errors/error-multiple-interfaces.4.good +++ b/test/library/standard/Sort/errors/error-multiple-interfaces.4.good @@ -1 +1 @@ -error-multiple-interfaces.chpl:16: error: The comparator R4 should only implement one sort comparator interface. +error-multiple-interfaces.chpl:32: error: The comparator R4 should only implement one sort comparator interface. diff --git a/test/library/standard/Sort/errors/error-multiple-interfaces.chpl b/test/library/standard/Sort/errors/error-multiple-interfaces.chpl index c453b19ec677..65382daedf1f 100644 --- a/test/library/standard/Sort/errors/error-multiple-interfaces.chpl +++ b/test/library/standard/Sort/errors/error-multiple-interfaces.chpl @@ -8,9 +8,25 @@ interface keyComparator { } var A = [1,2,3,4]; -record R1: keyPartComparator, relativeComparator { } -record R2: keyPartComparator, keyComparator { } -record R3: relativeComparator, keyComparator { } -record R4: keyComparator, keyPartComparator, relativeComparator { } +record R1: keyPartComparator, relativeComparator { + proc compare(a: int, b: int) { + return a - b; + } +} +record R2: keyPartComparator, keyComparator { + proc key(a: int) { + return a; + } +} +record R3: relativeComparator, keyComparator { + proc key(a: int) { + return a; + } +} +record R4: keyComparator, keyPartComparator, relativeComparator { + proc key(a: int) { + return a; + } +} sort(A, comparator=new comparator()); diff --git a/test/library/standard/Sort/errors/error-multiple-interfaces.compopts b/test/library/standard/Sort/errors/error-multiple-interfaces.compopts index 26d3d738ec3f..8626dc184a90 100644 --- a/test/library/standard/Sort/errors/error-multiple-interfaces.compopts +++ b/test/library/standard/Sort/errors/error-multiple-interfaces.compopts @@ -1,4 +1,4 @@ -scomparator=R1 -scomparator=R2 -scomparator=R3 --scomparator=R4 +-scomparator=R4 diff --git a/test/library/standard/Sort/errors/errors-compare.compopts b/test/library/standard/Sort/errors/errors-compare.compopts index 11a4a9f3ff5c..84306ca7102f 100644 --- a/test/library/standard/Sort/errors/errors-compare.compopts +++ b/test/library/standard/Sort/errors/errors-compare.compopts @@ -1,3 +1,4 @@ -scomparator=boolean # errors-compare.boolean.good -scomparator=rec # errors-compare.rec.good -scomparator=str # errors-compare.str.good +-scomparator=noKeyPart # errors-compare.noKeyPart.good diff --git a/test/library/standard/Sort/errors/errors-compare.noKeyPart.good b/test/library/standard/Sort/errors/errors-compare.noKeyPart.good new file mode 100644 index 000000000000..55b152cb71e5 --- /dev/null +++ b/test/library/standard/Sort/errors/errors-compare.noKeyPart.good @@ -0,0 +1,2 @@ +errors-compare.chpl:9: In function 'main': +errors-compare.chpl:17: error: The comparator noKeyPart implements the keyPartComparator interface, but the keyPart method is not implemented From cdfe707682ac1a586f16c28c24eda462cc6b1b9a Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Tue, 27 Aug 2024 17:21:07 -0700 Subject: [PATCH 15/22] fix script Signed-off-by: Jade Abraham --- .../unstableWarningAnonymizer/unstableAnonScript.chpl | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tools/unstableWarningAnonymizer/unstableAnonScript.chpl b/tools/unstableWarningAnonymizer/unstableAnonScript.chpl index 5654a1f15876..e7f42469b01e 100644 --- a/tools/unstableWarningAnonymizer/unstableAnonScript.chpl +++ b/tools/unstableWarningAnonymizer/unstableAnonScript.chpl @@ -65,6 +65,7 @@ import Set.set; import Sort; import Regex.regex; import ArgumentParser; +use Sort only relativeComparator; proc countUniqueWarnings(ref warningsMap: map(string, ?t), inputFileReader: IO.fileReader(?) @@ -165,10 +166,9 @@ inline proc csvPrintArr(arr: [] (string, int, int), } // Comparator to sort our array representation of the map -// by the number of occurences of each warning -record occurenceComparator {} -occurenceComparator implements relativeComparator -proc occurenceComparator.compare(a: (string, int, int), b: (string, int, int)){ +// by the number of occurrences of each warning +record occurrenceComparator: relativeComparator {} +proc occurrenceComparator.compare(a: (string, int, int), b: (string, int, int)){ return b[1] - a[1]; // Reverse sort } @@ -184,7 +184,7 @@ proc convertMapToArray(const m: map(string, ?t), sorted: bool, topX: int) a = (key, m[key][0], m[key][1].size); } if sorted { - var comp: occurenceComparator; + var comp: occurrenceComparator; Sort.sort(arr, comparator=comp); } else { var comp: warningComparator; From d1d76f8e9392abbd842c0da02d2c71a450ae2b26 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Wed, 28 Aug 2024 14:36:25 -0700 Subject: [PATCH 16/22] add unstable test Signed-off-by: Jade Abraham --- test/unstable/sortInterfaces.chpl | 8 ++++++++ test/unstable/sortInterfaces.good | 4 ++++ 2 files changed, 12 insertions(+) create mode 100644 test/unstable/sortInterfaces.chpl create mode 100644 test/unstable/sortInterfaces.good diff --git a/test/unstable/sortInterfaces.chpl b/test/unstable/sortInterfaces.chpl new file mode 100644 index 000000000000..ec337caa75dd --- /dev/null +++ b/test/unstable/sortInterfaces.chpl @@ -0,0 +1,8 @@ +use Sort; +record R1: relativeComparator { } + +record R2 { } +R2 implements keyPartComparator; + +record R3 { } +R3 implements sortComparator; diff --git a/test/unstable/sortInterfaces.good b/test/unstable/sortInterfaces.good new file mode 100644 index 000000000000..bd84a4179fdc --- /dev/null +++ b/test/unstable/sortInterfaces.good @@ -0,0 +1,4 @@ +sortInterfaces.chpl:1: warning: The Sort module interface is unstable +sortInterfaces.chpl:5: warning: keyPartComparator is not yet stable +sortInterfaces.chpl:8: warning: sortComparator is not yet stable +sortInterfaces.chpl:2: warning: relativeComparator is not yet stable From 28f3cce299a3865c7c5833a2253f85f7d477cc8b Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Wed, 28 Aug 2024 14:36:35 -0700 Subject: [PATCH 17/22] expand interface docs Signed-off-by: Jade Abraham --- modules/standard/Sort.chpl | 75 ++++++++++++++++++++++++++++++++------ 1 file changed, 63 insertions(+), 12 deletions(-) diff --git a/modules/standard/Sort.chpl b/modules/standard/Sort.chpl index 6bee5f5cc224..1a81aae4379b 100644 --- a/modules/standard/Sort.chpl +++ b/modules/standard/Sort.chpl @@ -3439,20 +3439,61 @@ private inline proc reverseKeyPartStatus(status: keyPartStatus): keyPartStatus d private inline proc reverseKeyPartStatus(status) do return -status; -// TODO: chpldoc will not render this yet :( // TODO: this is a hack to workaround issues with interfaces -// TODO: @unstable("keyPartComparator is not yet stable") +/* + The keyPartComparator interface defines how a comparator should sort parts of + a key, by defining :proc:`~keyPartComparator.Self.keyPart`. This is used for + certain sort algorithms. If :proc:`~keyPartComparator.Self.keyPart` is not + appropriate, the sort implementation may use + :proc:`~keyPartComparator.Self.compare` instead. +*/ +@unstable("keyPartComparator is not yet stable") interface keyPartComparator { + /* - proc Self.keyPart(elt, i: int): (keyPartStatus, integral); + A ``keyPart(elt, i)`` method returns *parts* of key value at a time. This + interface supports radix sorting for variable length data types, such as + strings. It accepts two arguments: + + * ``elt`` is the element being sorted + * ``i`` is the part number of the key requested, starting from 0 - // return type must be signed integral - proc Self.compare(x, y: x.type) { + A ``keyPart`` method should return a tuple consisting of *section* and a *part*. + + * The *section* must be of type :type:`keyPartStatus`. It indicates when the + end of ``elt`` has been reached and in that event how it should be sorted + relative to other array elements. + + * The *part* can be any signed or unsigned integral type and can contain any + value. The *part* will be ignored unless the *section* returned is + :enumconstant:`keyPartStatus.returned`. + + :arg elt: the element being sorted + :arg i: the part number requested + :returns: ``(section, part)`` where ``section`` is a :type:`keyPartStatus` + and ``part`` is an integral type. + */ + pragma "docs only" + proc Self.keyPart(elt, i: int): (keyPartStatus, integral); // due to current limitations this signature is only for chpldoc + + /* + Defines a comparison between two elements of the same type. This method is + not required to be implemented by comparator's that implement the + :interface:`keyPartComparator` interface. + + :arg x: the first element to compare + :arg y: the second element to compare + :returns: -1 if ``x`` should be sorted before ``y``, + 1 if ``x`` should be sorted after ``y``, + and 0 if ``x`` and ``y`` are equal + :rtype: a signed integral + */ + pragma "docs only" + proc Self.compare(x, y: x.type) { // due to current limitations this signature is only for chpldoc if x < y then return -1; else if y < x then return 1; else return 0; } - */ } private proc comparatorImplementsKeyPart(cmp) param do return __primitive("implements interface", cmp, keyPartComparator) != 2; @@ -3460,14 +3501,24 @@ private proc comparatorImplementsKeyPart(cmp) param do @chpldoc.nodoc config param useKeyPartStatus = false; -// TODO: chpldoc will not render this yet :( // TODO: this is a hack to workaround issues with interfaces -// TODO: @unstable("relativeComparator is not yet stable") +/* + The relativeComparator interface defines a comparison between two elements +*/ +@unstable("relativeComparator is not yet stable") interface relativeComparator { /* - // return type must be signed integral - proc Self.compare(x, y: x.type); + Defines a comparison between two elements of the same type. + + :arg x: the first element to compare + :arg y: the second element to compare + :returns: -1 if ``x`` should be sorted before ``y``, + 1 if ``x`` should be sorted after ``y``, + and 0 if ``x`` and ``y`` are equal + :rtype: a signed integral */ + pragma "docs only" + proc Self.compare(x, y: x.type); // due to current limitations this signature is only for chpldoc } private proc comparatorImplementsRelative(cmp) param do return __primitive("implements interface", cmp, relativeComparator) != 2; @@ -3477,8 +3528,8 @@ private proc comparatorImplementsRelative(cmp) param do // This cannot be represented in Chapel today, but we still want to // reserve the identifier. // See https://github.com/chapel-lang/chapel/issues/25554. -// TODO: this should be unstable, but can't apply attributes to interfaces -// @unstable("sortComparator is not yet stable") +@chpldoc.nodoc // keep this nodoc since its not implemented yet +@unstable("sortComparator is not yet stable") interface sortComparator { } /* Default comparator used in sort functions.*/ From 9445ea2337bc21661bac5a0a466706ff66cc18cb Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Wed, 28 Aug 2024 15:16:42 -0700 Subject: [PATCH 18/22] work around https://github.com/chapel-lang/chapel/issues/25838 Signed-off-by: Jade Abraham --- test/studies/comd/elegant/arrayOfStructs/util/Simulation.chpl | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/studies/comd/elegant/arrayOfStructs/util/Simulation.chpl b/test/studies/comd/elegant/arrayOfStructs/util/Simulation.chpl index c8c8118c5448..25544b8de8a1 100644 --- a/test/studies/comd/elegant/arrayOfStructs/util/Simulation.chpl +++ b/test/studies/comd/elegant/arrayOfStructs/util/Simulation.chpl @@ -392,11 +392,12 @@ proc pbc() { proc sortAtomsInCell() { use Sort; - record cmp: relativeComparator { + record cmp { proc compare(a, b) { return a.gid - b.gid; } } + cmp implements relativeComparator; const c = new cmp(); From 592b84521c9eb3644e186438e3bd74061a96a21c Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Wed, 28 Aug 2024 15:18:41 -0700 Subject: [PATCH 19/22] fix silly typo Signed-off-by: Jade Abraham --- test/deprecated/Sort/useRelativeComparator.chpl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/deprecated/Sort/useRelativeComparator.chpl b/test/deprecated/Sort/useRelativeComparator.chpl index 3224cd7b906d..e160f284bf95 100644 --- a/test/deprecated/Sort/useRelativeComparator.chpl +++ b/test/deprecated/Sort/useRelativeComparator.chpl @@ -16,7 +16,7 @@ record myCmp7{proc compare(x,y) do return if x Date: Wed, 28 Aug 2024 15:18:50 -0700 Subject: [PATCH 20/22] fix misspelled filename Signed-off-by: Jade Abraham --- .../deprecated/Sort/useRelativeComparator.good | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/test/deprecated/Sort/useRelativeComparator.good b/test/deprecated/Sort/useRelativeComparator.good index 446e8c9fa97f..010dafea93b1 100644 --- a/test/deprecated/Sort/useRelativeComparator.good +++ b/test/deprecated/Sort/useRelativeComparator.good @@ -1,15 +1,15 @@ -useRelativeCompator.chpl:44: warning: Defining a comparator with a 'compare' method without implementing the relativeComparator interface is deprecated. Please implement the relativeComparator interface (i.e. 'record myCmp1: relativeComparator'). -useRelativeCompator.chpl:45: warning: Defining a comparator with a 'compare' method without implementing the relativeComparator interface is deprecated. Please implement the relativeComparator interface (i.e. 'record myCmp2: relativeComparator'). -useRelativeCompator.chpl:46: warning: Defining a comparator with a 'compare' method without implementing the relativeComparator interface is deprecated. Please implement the relativeComparator interface (i.e. 'record myCmp3: relativeComparator'). -useRelativeCompator.chpl:49: warning: Defining a comparator with a 'compare' method without implementing the relativeComparator interface is deprecated. Please implement the relativeComparator interface (i.e. 'record myCmp4: relativeComparator'). -useRelativeCompator.chpl:50: warning: Defining a comparator with a 'compare' method without implementing the relativeComparator interface is deprecated. Please implement the relativeComparator interface (i.e. 'record myCmp5: relativeComparator'). -useRelativeCompator.chpl:51: warning: Defining a comparator with a 'compare' method without implementing the relativeComparator interface is deprecated. Please implement the relativeComparator interface (i.e. 'record myCmp6: relativeComparator'). -useRelativeCompator.chpl:54: warning: Defining a comparator with a 'compare' method without implementing the relativeComparator interface is deprecated. Please implement the relativeComparator interface (i.e. 'record myCmp7: relativeComparator'). +useRelativeComparator.chpl:44: warning: Defining a comparator with a 'compare' method without implementing the relativeComparator interface is deprecated. Please implement the relativeComparator interface (i.e. 'record myCmp1: relativeComparator'). +useRelativeComparator.chpl:45: warning: Defining a comparator with a 'compare' method without implementing the relativeComparator interface is deprecated. Please implement the relativeComparator interface (i.e. 'record myCmp2: relativeComparator'). +useRelativeComparator.chpl:46: warning: Defining a comparator with a 'compare' method without implementing the relativeComparator interface is deprecated. Please implement the relativeComparator interface (i.e. 'record myCmp3: relativeComparator'). +useRelativeComparator.chpl:49: warning: Defining a comparator with a 'compare' method without implementing the relativeComparator interface is deprecated. Please implement the relativeComparator interface (i.e. 'record myCmp4: relativeComparator'). +useRelativeComparator.chpl:50: warning: Defining a comparator with a 'compare' method without implementing the relativeComparator interface is deprecated. Please implement the relativeComparator interface (i.e. 'record myCmp5: relativeComparator'). +useRelativeComparator.chpl:51: warning: Defining a comparator with a 'compare' method without implementing the relativeComparator interface is deprecated. Please implement the relativeComparator interface (i.e. 'record myCmp6: relativeComparator'). +useRelativeComparator.chpl:54: warning: Defining a comparator with a 'compare' method without implementing the relativeComparator interface is deprecated. Please implement the relativeComparator interface (i.e. 'record myCmp7: relativeComparator'). $CHPL_HOME/modules/standard/Sort.chpl:nnnn: In iterator 'sorted': $CHPL_HOME/modules/standard/Sort.chpl:nnnn: warning: Defining a comparator with a 'compare' method without implementing the relativeComparator interface is deprecated. Please implement the relativeComparator interface (i.e. 'record myCmp8: relativeComparator'). - useRelativeCompator.chpl:55: called as sorted(x: [domain(1,int(64),one)] int(64), comparator: myCmp8) + useRelativeComparator.chpl:55: called as sorted(x: [domain(1,int(64),one)] int(64), comparator: myCmp8) note: generic instantiations are underlined in the above callstack -useRelativeCompator.chpl:56: note: Defining a comparator with a 'compare' method without implementing the relativeComparator interface is deprecated. Please implement the relativeComparator interface (i.e. 'record myCmp9: relativeComparator'). +useRelativeComparator.chpl:56: note: Defining a comparator with a 'compare' method without implementing the relativeComparator interface is deprecated. Please implement the relativeComparator interface (i.e. 'record myCmp9: relativeComparator'). isSorted before false isSorted after true isSorted before false From 7323b9ccc01e84527370adaa471b6da7e87a5419 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Wed, 28 Aug 2024 15:20:55 -0700 Subject: [PATCH 21/22] update good file to reflect improved errors Signed-off-by: Jade Abraham --- test/deprecated/Sort/useRelativeComparator.good | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/deprecated/Sort/useRelativeComparator.good b/test/deprecated/Sort/useRelativeComparator.good index 010dafea93b1..4bb8279ce213 100644 --- a/test/deprecated/Sort/useRelativeComparator.good +++ b/test/deprecated/Sort/useRelativeComparator.good @@ -8,8 +8,7 @@ useRelativeComparator.chpl:54: warning: Defining a comparator with a 'compare' m $CHPL_HOME/modules/standard/Sort.chpl:nnnn: In iterator 'sorted': $CHPL_HOME/modules/standard/Sort.chpl:nnnn: warning: Defining a comparator with a 'compare' method without implementing the relativeComparator interface is deprecated. Please implement the relativeComparator interface (i.e. 'record myCmp8: relativeComparator'). useRelativeComparator.chpl:55: called as sorted(x: [domain(1,int(64),one)] int(64), comparator: myCmp8) -note: generic instantiations are underlined in the above callstack -useRelativeComparator.chpl:56: note: Defining a comparator with a 'compare' method without implementing the relativeComparator interface is deprecated. Please implement the relativeComparator interface (i.e. 'record myCmp9: relativeComparator'). +useRelativeComparator.chpl:56: warning: Defining a comparator with a 'compare' method without implementing the relativeComparator interface is deprecated. Please implement the relativeComparator interface (i.e. 'record myCmp9: relativeComparator'). isSorted before false isSorted after true isSorted before false From 86e567459c7bce55dc9cf23b668421e46aef13a3 Mon Sep 17 00:00:00 2001 From: Jade Abraham Date: Thu, 29 Aug 2024 14:22:32 -0700 Subject: [PATCH 22/22] apply feedback Signed-off-by: Jade Abraham --- modules/standard/Sort.chpl | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/modules/standard/Sort.chpl b/modules/standard/Sort.chpl index 1a81aae4379b..46230cdb5dd9 100644 --- a/modules/standard/Sort.chpl +++ b/modules/standard/Sort.chpl @@ -458,14 +458,23 @@ proc chpl_check_comparator(comparator, const keydata = comparator.key(data); type keytype = keydata.type; if !(canResolve("<", keydata, keydata)) then - compilerError(errorDepth=errorDepth, "The key method in ", comparator.type:string, " must return an object that supports the '<' function when used with ", eltType:string, " elements"); + compilerError(errorDepth=errorDepth, + "The key method in ", + comparator.type:string, + " must return an object that supports the '<' function when used with ", + eltType:string, + " elements"); // Check that there isn't also a compare or keyPart if canResolveMethod(comparator, "compare", data, data) { - compilerError(errorDepth=errorDepth, comparator.type:string, " contains both a key method and a compare method"); + compilerError(errorDepth=errorDepth, + comparator.type:string, + " contains both a key method and a compare method"); } if canResolveMethod(comparator, "keyPart", data, 0) { - compilerError(errorDepth=errorDepth, comparator.type:string, " contains both a key method and a keyPart method"); + compilerError(errorDepth=errorDepth, + comparator.type:string, + " contains both a key method and a keyPart method"); } } else if canResolveMethod(comparator, "compare", data, data) { @@ -3478,7 +3487,7 @@ interface keyPartComparator { /* Defines a comparison between two elements of the same type. This method is - not required to be implemented by comparator's that implement the + not required to be implemented by comparators that implement the :interface:`keyPartComparator` interface. :arg x: the first element to compare