respond to reviewer feedback

Signed-off-by: Jade Abraham <[email protected]>

compiler/resolution/cullOverReferences.cpp

@@ -460,66 +460,68 @@ void markSymbolConst(Symbol* sym)
     sym->addFlag(FLAG_CONST);
   }
 }
-static
-void markSymbolNotConst(Symbol* sym)
-{
-  ArgSymbol* arg = toArgSymbol(sym);
 
-  // This could set sym->qual to e.g. QUAL_REF
-  // but currently QUAL_REF is used both for
-  // ref-with-unknown-constness and ref-not-const,
-  // so we can just leave it alone.
-  INT_ASSERT(!sym->qualType().isConst());
-  if (arg && arg->intent == INTENT_REF_MAYBE_CONST) {
+static void maybeIssueRefMaybeConstWarning(ArgSymbol* arg) {
+  bool isArgThis = arg->hasFlag(FLAG_ARG_THIS);
+  // this is still being used for tuples assignment
+  bool fromPragma = arg->hasFlag(FLAG_INTENT_REF_MAYBE_CONST_FORMAL);
 
-    bool isArgThis = arg->hasFlag(FLAG_ARG_THIS);
-    // this is still being used for tuples assignment
-    bool fromPragma = arg->hasFlag(FLAG_INTENT_REF_MAYBE_CONST_FORMAL);
+  bool isCompilerGenerated = false;
 
-    bool isCompilerGenerated = false;
+  bool isTaskIntent = false;
+  if (FnSymbol* fn = arg->getFunction()) {
+    isTaskIntent = fn->hasEitherFlag(FLAG_COBEGIN_OR_COFORALL, FLAG_BEGIN);
 
-    bool isTaskIntent = false;
-    if (FnSymbol* fn = arg->getFunction()) {
-      isTaskIntent = fn->hasEitherFlag(FLAG_COBEGIN_OR_COFORALL, FLAG_BEGIN);
+    isCompilerGenerated = fn->hasFlag(FLAG_COMPILER_GENERATED);
+  }
 
-      isCompilerGenerated = fn->hasFlag(FLAG_COMPILER_GENERATED);
-    }
+  // we have full control here, but this should be ok
+  bool isOuter = arg->hasFlag(FLAG_OUTER_VARIABLE);
 
-    // we have full control here, but this should be ok
-    bool isOuter = arg->hasFlag(FLAG_OUTER_VARIABLE);
+  bool notImplementedYetOptOut = isArgThis || isTaskIntent;
 
-    bool notImplementedYetOptOut = isArgThis || isTaskIntent;
 
+  bool shouldWarn = !notImplementedYetOptOut && !isOuter && !fromPragma && !isCompilerGenerated;
 
-    bool shouldWarn = !notImplementedYetOptOut && !isOuter && !fromPragma && !isCompilerGenerated;
+  if (shouldWarn) {
+    IntentTag defaultIntent = blankIntentForType(arg->type);
+    // if default intent is not ref-maybe-const, do nothing
+    if(defaultIntent != INTENT_REF_MAYBE_CONST) shouldWarn = false;
+  }
+
+  if (shouldWarn) {
 
-    if (shouldWarn) {
-      IntentTag defaultIntent = blankIntentForType(arg->type);
-      // if default intent is not ref-maybe-const, do nothing
-      if(defaultIntent != INTENT_REF_MAYBE_CONST) shouldWarn = false;
+    const char* argName = nullptr;
+    char argBuffer[64];
+    if (!arg->hasFlag(FLAG_EXPANDED_VARARGS)) {
+      argName = arg->name;
+    } else {
+      int varArgNum;
+      int ret = sscanf(arg->name, "_e%d_%63s", &varArgNum, argBuffer);
+      CHPL_ASSERT(ret == 2);
+      argName = argBuffer;
     }
 
-    if (shouldWarn) {
+    USR_WARN(arg,
+              "inferring a default intent to be 'ref' is deprecated "
+              "- please use an explicit intent for the argument '%s'",
+              argName);
+  }
+}
 
-      const char* intentName = isTaskIntent ? "task" : (isArgThis ? "this" : "argument");
+static
+void markSymbolNotConst(Symbol* sym)
+{
+  ArgSymbol* arg = toArgSymbol(sym);
 
-      const char* argName = nullptr;
-      char argBuffer[64];
-      if (!arg->hasFlag(FLAG_EXPANDED_VARARGS)) {
-        argName = arg->name;
-      } else {
-        int varArgNum;
-        int ret = sscanf(arg->name, "_e%d_%63s", &varArgNum, argBuffer);
-        CHPL_ASSERT(ret == 2);
-        argName = argBuffer;
-      }
+  // This could set sym->qual to e.g. QUAL_REF
+  // but currently QUAL_REF is used both for
+  // ref-with-unknown-constness and ref-not-const,
+  // so we can just leave it alone.
+  INT_ASSERT(!sym->qualType().isConst());
+  if (arg && arg->intent == INTENT_REF_MAYBE_CONST) {
 
-      USR_WARN(arg,
-               "inferring a default %s intent to be 'ref' for '%s' is deprecated "
-               "- please use an explicit intent",
-               intentName,
-               argName);
-    }
+    maybeIssueRefMaybeConstWarning(arg);
 
     arg->intent = INTENT_REF;
   }

modules/internal/ChapelDomain.chpl

@@ -2110,7 +2110,7 @@ module ChapelDomain {
       return _value.dsiBulkAdd(inds, dataSorted, isUnique, addOn);
     }
 
-    @unstable("bulkAdd() is subject to change in the future.")
+    @unstable("bulkAddNoPreserveInds() is subject to change in the future.")
     proc ref bulkAddNoPreserveInds(ref inds: [] _value.rank*_value.idxType,
         dataSorted=false, isUnique=false, addOn=nilLocale)
         where this.isSparse() && _value.rank>1 {

test/arrays/ferguson/semantic-examples/1.5-return-alias-slice-global.chpl

@@ -8,11 +8,12 @@ proc f() {
   return slice;
 }
 
-proc g(in x) {
+proc g(ref x) {
   x[2] = one;
 }
 
-g(f());
+var t = f();
+g(t);
 writeln(A[2].x);
 
 // Does this program output 0 or 1?

test/deprecated/ref-maybe-const.good

@@ -1,5 +1,5 @@
-ref-maybe-const.chpl:7: warning: inferring a default argument intent to be 'ref' for 'A' is deprecated - please use an explicit intent
-ref-maybe-const.chpl:12: warning: inferring a default argument intent to be 'ref' for 'args' is deprecated - please use an explicit intent
+ref-maybe-const.chpl:7: warning: inferring a default intent to be 'ref' is deprecated - please use an explicit intent for the argument 'A'
+ref-maybe-const.chpl:12: warning: inferring a default intent to be 'ref' is deprecated - please use an explicit intent for the argument 'args'
 A, 1 2 3 4 5 6 7 8 9 10
 B, 1 2 3 4 5 6 7 8 9 10
 A, 1 1 1 1 1 1 1 1 1 1

test/release/examples/primers/arrays.chpl

@@ -158,9 +158,10 @@ negateAndPrintArr(B);
 
 //
 // Arrays are passed to routines by constant reference (``const ref``) by
-// default, so for the modification to ``X`` in procedure
-// ``negateAndPrintArr()`` to be legal and reflected back to the actual argument
-// ``B``  the explicit ``ref`` must be used.
+// default, which does not allow them to be modified within the routine.
+// The above procedure ``negateAndPrintArr()`` must use a non-constant
+// reference intent (``ref``) explicitly, so that its modifications of ``X``
+// are both allowed and reflected in the actual argument ``B``:
 //
 
 writeln("After calling negateAndPrintArr, B is:\n", B, "\n");

test/release/examples/primers/learnChapelInYMinutes.chpl

@@ -497,7 +497,7 @@ Procedures
 ----------
 */
 
-// Chapel procedures have similar syntax functions in other languages.
+// Chapel procedures have similar syntax to functions in other languages.
 proc fibonacci(n : int) : int {
   if n <= 1 then return n;
   return fibonacci(n-1) + fibonacci(n-2);

test/studies/cholesky/marybeth/chol.chpl

@@ -27,7 +27,7 @@ proc main() {
 
   chol(A);
 
-writeln("Factored Matrix:");
+  writeln("Factored Matrix:");
   writelower(A);
   writeln();
 }