Follow up on review feedback

Signed-off-by: Vassily Litvinov <vasslitvinov@users.noreply.github.com>

compiler/AST/AggregateType.cpp

@@ -923,23 +923,24 @@ static void replaceStridesWithStridableSE(SymExpr* se) {
 // `rect dom or arr class(stridable=...)` to `(strides=...)`
 static void checkRangeDeprecations(AggregateType* at, NamedExpr* ne,
                                    Symbol*& field) {
-  if ((!strcmp(ne->name, "boundedType") || !strcmp(ne->name, "stridable"))
-      && at->symbol->hasFlag(FLAG_RANGE))
+  bool isBoundedType = !strcmp(ne->name, "boundedType");
+  bool isStridable   = !strcmp(ne->name, "stridable");
+  if ((isBoundedType || isStridable) && at->symbol->hasFlag(FLAG_RANGE))
   {
-   if (!strcmp(ne->name, "boundedType")) {
-    USR_WARN(ne,
-      "range.boundedType is deprecated; please use '.bounds' instead");
-    field = at->getField("bounds");
-   }
-   else { // "stridable"
+    if (isBoundedType) {
+      USR_WARN(ne,
+        "range.boundedType is deprecated; please use '.bounds' instead");
+      field = at->getField("bounds");
+    }
+    else { // "stridable"
 #if 0 //RSDW
-    USR_WARN(ne,
-      "range.stridable is deprecated; please use '.strides' instead");
+      USR_WARN(ne,
+        "range.stridable is deprecated; please use '.strides' instead");
 #endif
-    field = at->getField("strides");
-    replaceStridesWithStridableSE(toSymExpr(ne->actual));
-   }
-  } else if (!strcmp(ne->name, "stridable")) {
+      field = at->getField("strides");
+      replaceStridesWithStridableSE(toSymExpr(ne->actual));
+    }
+  } else if (isStridable) {
     if (AggregateType* base = baseRectDsiParent(at)) {
 #if 0 //RSDW
       USR_WARN(ne,
@@ -2221,9 +2222,8 @@ AggregateType* baseRectDsiParent(AggregateType* ag) {
   if (ag->aggregateTag != AGGREGATE_CLASS)
     return nullptr;
 
-  while (AggregateType* parentAG = ag->dispatchParents.n == 0 ?
-           nullptr : ag->dispatchParents.v[0])
-  {
+  while (ag->dispatchParents.n > 0) {
+    AggregateType* parentAG = ag->dispatchParents.v[0];
     TypeSymbol* psym = parentAG->symbol;
 
     if (psym->hasFlag(FLAG_OBJECT_CLASS))

compiler/AST/wellknown.cpp

@@ -96,7 +96,7 @@ FnSymbol *gChplBuildLocaleId;
 void gatherIteratorTags() {
   forv_Vec(TypeSymbol, ts, gTypeSymbols) {
     if (strcmp(ts->name, iterKindTypename) == 0
-        || strcmp(ts->name, "strideKind")  == 0) {
+        || strcmp(ts->name, strideKindTypename)  == 0) {
       if (EnumType* enumType = toEnumType(ts->type)) {
         for_alist(expr, enumType->constants) {
           if (DefExpr* def = toDefExpr(expr)) {
@@ -111,10 +111,10 @@ void gatherIteratorTags() {
             } else if (strcmp(name, iterKindStandaloneTagname) == 0) {
               gStandaloneTag = def->sym;
 
-            } else if (strcmp(name, "one")                     == 0) {
+            } else if (strcmp(name, strideKindOneTagname)      == 0) {
               gStrideOne     = def->sym;
 
-            } else if (strcmp(name, "any")                     == 0) {
+            } else if (strcmp(name, strideKindAnyTagname)      == 0) {
               gStrideAny     = def->sym;
             }
           }

compiler/include/misc.h

@@ -82,6 +82,9 @@
 #define iterKindFollowerTagname   "follower"
 #define iterKindStandaloneTagname "standalone"
 #define iterFollowthisArgname     "followThis"
+#define strideKindTypename        "strideKind"
+#define strideKindOneTagname      "one"
+#define strideKindAnyTagname      "any"
 
 #define tupleInitName "chpl__init_tuple"
 

compiler/include/passes.h

@@ -155,8 +155,8 @@ void checkUseBeforeDefs(FnSymbol* fn);
 void addMentionToEndOfStatement(Expr* node, CallExpr* existingEndOfStatement);
 Expr* partOfNonNormalizableExpr(Expr* expr);
 // support for deprecation by Vass in 1.31 to implement #17131
-bool replacedStridable(CallExpr* parentCall, const char* name,
-                       UnresolvedSymExpr* use);
+bool tryReplaceStridable(CallExpr* parentCall, const char* name,
+                         UnresolvedSymExpr* use);
 
 // parallel.cpp
 Type* getOrMakeRefTypeDuringCodegen(Type* type);

compiler/passes/normalize.cpp

@@ -764,9 +764,8 @@ void checkUseBeforeDefs(FnSymbol* fn) {
           if (isFnSymbol(fn->defPoint->parentSymbol) == false) {
             const char* name = use->unresolved;
 
-            if (replacedStridable(call, name, use))
-              ; // handled
-            else
+            if (tryReplaceStridable(call, name, use))
+              continue;
 
             // Only complain one time
             if (undeclared.find(name) == undeclared.end()) {
@@ -891,8 +890,8 @@ static bool replaceWithStridableCall(Symbol* stridesField, Expr* use) {
 // Supports deprecation by Vass in 1.31 to implement #17131.
 // chpl__buildDomainRuntimeType(..., stridable) -->
 // chpl__buildDomainRuntimeType(..., strides) if we are in a DSI class.
-bool replacedStridable(CallExpr* parentCall, const char* name,
-                       UnresolvedSymExpr* use)
+bool tryReplaceStridable(CallExpr* parentCall, const char* name,
+                         UnresolvedSymExpr* use)
 {
   if (strcmp(name, "stridable")) return false;
 

compiler/passes/scopeResolve.cpp

@@ -856,25 +856,25 @@ static bool callSpecifiesClassKind(CallExpr* call) {
 }
 
 // Supports deprecation by Vass in 1.31 to implement #17131.
-static bool replacedStridableSR(const char* name, UnresolvedSymExpr* use) {
+static bool tryReplaceStridableSR(const char* name, UnresolvedSymExpr* use) {
   CallExpr* pCall = toCallExpr(use->parentExpr);
   if (pCall == nullptr)
     if (NamedExpr* pNamed = toNamedExpr(use->parentExpr))
       pCall = toCallExpr(pNamed->parentExpr);
-  return replacedStridable(pCall, name, use);
+  return tryReplaceStridable(pCall, name, use);
 }
 
 // Supports deprecation by Vass in 1.31 to implement #17131.
 // Sometimes lookupAndCount() will return a type method 'stridable'.
 // This is more likely a bug. So we steamroll over it.
-static bool replacedStridableSR(const char* name, UnresolvedSymExpr* use,
+static bool tryReplaceStridableSR(const char* name, UnresolvedSymExpr* use,
                                 Symbol* sym) {
   if (!strcmp(name, "stridable"))
     if (FnSymbol* symFn = toFnSymbol(sym))
       if (symFn->thisTag == INTENT_TYPE)
         if (symFn->getModule()->modTag == MOD_INTERNAL)
           // ignore 'sym'
-          return replacedStridableSR(name, use);
+          return tryReplaceStridableSR(name, use);
   return false;
 }
 
@@ -915,10 +915,12 @@ static astlocT* resolveUnresolvedSymExpr(UnresolvedSymExpr* usymExpr,
   Symbol* sym = lookupAndCount(name, usymExpr, nSymbols, returnRename,
                                &renameLoc);
   if (sym != NULL) {
-   if (!replacedStridableSR(name, usymExpr, sym))
-    resolveUnresolvedSymExpr(usymExpr, sym);
-  } else if (replacedStridableSR(name, usymExpr)) {
+    if (!tryReplaceStridableSR(name, usymExpr, sym))
+      resolveUnresolvedSymExpr(usymExpr, sym);
+
+  } else if (tryReplaceStridableSR(name, usymExpr)) {
     // handled
+
   } else {
     updateMethod(usymExpr);
   }

compiler/resolution/virtualDispatch.cpp

@@ -1273,7 +1273,15 @@ static void checkMethodsOverride() {
             if (erroredFunctions.count(eFn) == 0) {
               if (fn->hasFlag(FLAG_OVERRIDE)) {
                if (isDsiNewRectangularDom(fn)) {
-                // allow, for deprecation by Vass in 1.31 to implement #17131
+                // Allow, for deprecation by Vass in 1.31 to implement #17131.
+                // To support this deprecation, #22441 comments out
+                // BaseDist.dsiNewRectangularDom(), see an explanation in
+                // ChapelDistribution.chpl. However existing code overrides
+                // these methods and we expect to reinstate them once
+                // the deprecated features have been removed completely.
+                // To avoid forcing existing code to remove 'override'
+                // annotations then add them back, accept them for now
+                // despite that being technically incorrect.
                } else {
                 USR_FATAL_CONT(fn, "%s.%s override keyword present but "
                                     "no superclass method matches signature "

modules/dists/DSIUtil.chpl

@@ -265,11 +265,9 @@ private proc densiResult(arg: range(?), whole: range(?)) type
 // or with assert() (if false).
 //
 
-// would like 'whole: domain(?IT,?r,?)'
 proc densify(sub: domain, whole: domain, userErrors = true
              ) : densiResult(sub, whole)
 {
-
   type argtypes = (sub, whole).type;
   _densiCheck(sub.rank == whole.rank, argtypes);
   _densiIdxCheck(sub.idxType, whole.idxType,  argtypes);
@@ -346,6 +344,7 @@ proc densify(s: range(?,bounds=?B), w: range(?IT,?), userErrors=true
 }
 
 // w.strides==one
+// in this case densiResult(sArg,w) == range(IT,B,S)
 proc densify(sArg: range(?,bounds=?B,strides=?S), w: range(?IT,?,strides=strideKind.one), userErrors=true) : range(IT,B,S)
 {
   _densiEnsureBounded(sArg);

modules/internal/ChapelArray.chpl

@@ -787,8 +787,8 @@ module ChapelArray {
         return _value.dsiNewRectangularDom(rank, idxType, stridable, ranges2);
       }
 
-        compilerError("rectangular domains are not supported by",
-                      " the distribution ", this.type:string);
+      compilerError("rectangular domains are not supported by",
+                    " the distribution ", this.type:string);
     }
 
     proc newRectangularDom(param rank: int, type idxType,

modules/internal/ChapelDomain.chpl

@@ -723,9 +723,9 @@ module ChapelDomain {
 
     if a.isRectangular() && b.isRectangular() then
       if ! chpl_assignStrideIsSafe(a.strides, b.strides) then
-       compilerError("assigning to a domain with strideKind.",a.strides:string,
-                     " from a domain with strideKind.", b.strides:string,
-                     " without an explicit cast");
+        compilerError("assigning to a domain with strideKind.",a.strides:string,
+                      " from a domain with strideKind.", b.strides:string,
+                      " without an explicit cast");
 
     a._instance.dsiAssignDomain(b, lhsPrivate=false);
 
@@ -1458,13 +1458,14 @@ module ChapelDomain {
         compilerWarning("arrays and array slices with negatively-strided dimensions are currently unsupported and may lead to unexpected behavior; compile with -snoNegativeStrideWarnings to suppress this warning");
        } else
 */
-       if ! this.strides.isPositive() {
-        for s in chpl__tuplify(this.stride) do
-          if s < 0 {
-            warning("arrays and array slices with negatively-strided dimensions are currently unsupported and may lead to unexpected behavior; compile with -snoNegativeStrideWarnings to suppress this warning; the dimension(s) are: ", this.dsiDims());
-            break;
+        if ! this.strides.isPositive() {
+          for s in chpl__tuplify(this.stride) {
+            if s < 0 {
+              warning("arrays and array slices with negatively-strided dimensions are currently unsupported and may lead to unexpected behavior; compile with -snoNegativeStrideWarnings to suppress this warning; the dimension(s) are: ", this.dsiDims());
+              break;
+            }
           }
-       }
+        }
       }
     }
 

modules/internal/ChapelRange.chpl

@@ -1268,9 +1268,9 @@ module ChapelRange {
     if this.bounds == boundKind.both {
       return this.firstAsInt;
     } else {
-      if strides == strideKind.one {
+      if strides.isOne() {
         return chpl__idxToInt(lowBoundForIter(this));
-      } else if strides == strideKind.negOne {
+      } else if strides.isNegOne() {
         return chpl__idxToInt(highBoundForIter(this));
       } else {
         if hasPositiveStride() {
@@ -1323,9 +1323,9 @@ module ChapelRange {
     if bounds == boundKind.both {
       return this.lastAsInt;
     } else {
-      if strides == strideKind.one {
+      if strides.isOne() {
         return chpl__idxToInt(highBoundForIter(this));
-      } else if strides == strideKind.negOne {
+      } else if strides.isNegOne() {
         return chpl__idxToInt(lowBoundForIter(this));
       } else if hasPositiveStride() {
         return helpAlignHigh(chpl__idxToInt(highBoundForIter(this)), _alignment, _stride);
@@ -1690,10 +1690,10 @@ private inline proc rangeCastHelper(r, type t) throws {
       HaltWrappers.boundsCheckHalt("indexOrder -- Undefined on a range with ambiguous alignment.");
 
     if ! contains(ind) then return (-1):intIdxType;
-    if strides == strideKind.one {
+    if strides.isOne() {
       if this.hasLowBound() then
         return chpl__idxToInt(ind) - _low;
-    } else if strides == strideKind.negOne {
+    } else if strides.isNegOne() {
       if this.hasHighBound() then
         return _high - chpl__idxToInt(ind);
     } else {
@@ -3513,51 +3513,51 @@ private inline proc rangeCastHelper(r, type t) throws {
       if boundsChecking && this.hasLast() then
         HaltWrappers.zipLengthHalt("zippered iteration where a bounded range follows an unbounded iterator");
 
-     if ! newStrides.isPosNegOne() {      // aka !newStrides.hasParamStride()
-      const first  = this.orderToIndex(myFollowThis.first);
-      const stride = this.stride * myFollowThis.stride;
+      if ! newStrides.isPosNegOne() {      // aka !newStrides.hasParamStride()
+        const first  = this.orderToIndex(myFollowThis.first);
+        const stride = this.stride * myFollowThis.stride;
 
-      if isPositiveStride(newStrides, stride)
-      {
-        const r = first .. by stride:strType;
-        if debugChapelRange then
-          chpl_debug_writeln("Expanded range = ",r);
+        if isPositiveStride(newStrides, stride)
+        {
+          const r = first .. by stride:strType;
+          if debugChapelRange then
+            chpl_debug_writeln("Expanded range = ",r);
 
-        for i in r do
-          yield i;
-      }
-      else
-      {
-        const r = .. first by stride:strType;
-        if debugChapelRange then
-          chpl_debug_writeln("Expanded range = ",r);
-
-        for i in r do
-          yield i;
-      }
-     } else {  // newStrides.isPosNegOne()
-      // else-branch follows then-branch; here 'stride' is param 1 or -1
-      const first  = this.orderToIndex(myFollowThis.first);
-
-      if newStrides.isPositive()
-      {
-        const r = first ..;
-        if debugChapelRange then
-          chpl_debug_writeln("Expanded range = ",r);
+          for i in r do
+            yield i;
+        }
+        else
+        {
+          const r = .. first by stride:strType;
+          if debugChapelRange then
+            chpl_debug_writeln("Expanded range = ",r);
 
-        for i in r do
-          yield i;
-      }
-      else
-      {
-        const r = .. first by -1;
-        if debugChapelRange then
-          chpl_debug_writeln("Expanded range = ",r);
+          for i in r do
+            yield i;
+        }
+      } else {  // newStrides.isPosNegOne()
+        // else-branch follows then-branch; here 'stride' is param 1 or -1
+        const first  = this.orderToIndex(myFollowThis.first);
+
+        if newStrides.isPositive()
+        {
+          const r = first ..;
+          if debugChapelRange then
+            chpl_debug_writeln("Expanded range = ",r);
+
+          for i in r do
+            yield i;
+        }
+        else
+        {
+          const r = .. first by -1;
+          if debugChapelRange then
+            chpl_debug_writeln("Expanded range = ",r);
 
-        for i in r do
-          yield i;
+          for i in r do
+            yield i;
+        }
       }
-     }
     } // if myFollowThis.hasLast()
   }