Changeset: Replace output params with return values

This improves readability and reduces the chances of introducing a
bug.

src/node/utils/padDiff.js

@@ -273,7 +273,7 @@ PadDiff.prototype._createDeletionChangeset = function (cs, startAText, apool) {
   let curChar = 0;
   let curLineOpIter = null;
   let curLineOpIterLine;
-  const curLineNextOp = Changeset.newOp('+');
+  let curLineNextOp = Changeset.newOp('+');
 
   const unpacked = Changeset.unpack(cs);
   const csIter = Changeset.opIterator(unpacked.ops);
@@ -287,7 +287,7 @@ PadDiff.prototype._createDeletionChangeset = function (cs, startAText, apool) {
       let indexIntoLine = 0;
       let done = false;
       while (!done) {
-        curLineOpIter.next(curLineNextOp);
+        curLineNextOp = curLineOpIter.next();
         if (indexIntoLine + curLineNextOp.chars >= curChar) {
           curLineNextOp.chars -= (curChar - indexIntoLine);
           done = true;
@@ -307,7 +307,7 @@ PadDiff.prototype._createDeletionChangeset = function (cs, startAText, apool) {
       }
 
       if (!curLineNextOp.chars) {
-        curLineOpIter.next(curLineNextOp);
+        curLineNextOp = curLineOpIter.next();
       }
 
       const charsToUse = Math.min(numChars, curLineNextOp.chars);

src/static/js/Changeset.js

@@ -947,7 +947,7 @@ const textLinesMutator = (lines) => {
  * @param {string} in2 - second Op string
  * @param {Function} func - Callback that applies an operation to another operation. Will be called
  *     multiple times depending on the number of operations in `in1` and `in2`. `func` has signature
- *     `f(op1, op2, opOut)`:
+ *     `opOut = f(op1, op2)`:
  *       - `op1` is the current operation from `in1`. `func` is expected to mutate `op1` to
  *         partially or fully consume it, and MUST set `op1.opcode` to the empty string once `op1`
  *         is fully consumed. If `op1` is not fully consumed, `func` will be called again with the
@@ -956,9 +956,9 @@ const textLinesMutator = (lines) => {
  *         the empty string.
  *       - `op2` is the current operation from `in2`, to apply to `op1`. Has the same consumption
  *         and advancement semantics as `op1`.
- *       - `opOut` MUST be mutated to reflect the result of applying `op2` (before consumption) to
+ *       - `opOut` is the result of applying `op2` (before consumption) to `op1` (before
- *         `op1` (before consumption). If there is no result (perhaps `op1` and `op2` cancelled each
+ *         consumption). If there is no result (perhaps `op1` and `op2` cancelled each other out),
- *         other out), `opOut.opcode` MUST be set to the empty string.
+ *         either `opOut` must be nullish or `opOut.opcode` must be the empty string.
  * @returns {string} the integrated changeset
  */
 const applyZip = (in1, in2, func) => {
@@ -967,15 +967,11 @@ const applyZip = (in1, in2, func) => {
   const assem = exports.smartOpAssembler();
   const op1 = exports.newOp();
   const op2 = exports.newOp();
-  const opOut = exports.newOp();
   while (op1.opcode || iter1.hasNext() || op2.opcode || iter2.hasNext()) {
     if ((!op1.opcode) && iter1.hasNext()) iter1.next(op1);
     if ((!op2.opcode) && iter2.hasNext()) iter2.next(op2);
-    func(op1, op2, opOut);
+    const opOut = func(op1, op2);
-    if (opOut.opcode) {
+    if (opOut && opOut.opcode) assem.append(opOut);
-      assem.append(opOut);
-      opOut.opcode = '';
-    }
   }
   assem.endDocument();
   return assem.toString();
@@ -1171,11 +1167,11 @@ exports.composeAttributes = (att1, att2, resultIsMutation, pool) => {
  * @param {Op} attOp - The op from the sequence that is being operated on, either an attribution
  *     string or the earlier of two exportss being composed.
  * @param {Op} csOp -
- * @param {Op} opOut - Mutated to hold the result of applying `csOp` to `attOp`.
  * @param {AttributePool} pool - Can be null if definitely not needed.
+ * @returns {Op} The result of applying `csOp` to `attOp`.
  */
-const slicerZipperFunc = (attOp, csOp, opOut, pool) => {
+const slicerZipperFunc = (attOp, csOp, pool) => {
-  clearOp(opOut);
+  const opOut = exports.newOp();
   if (attOp.opcode === '-') {
     copyOp(attOp, opOut);
     attOp.opcode = '';
@@ -1263,6 +1259,7 @@ const slicerZipperFunc = (attOp, csOp, opOut, pool) => {
       }
     }
   }
+  return opOut;
 };
 
 /**
@@ -1275,9 +1272,7 @@ const slicerZipperFunc = (attOp, csOp, opOut, pool) => {
  */
 exports.applyToAttribution = (cs, astr, pool) => {
   const unpacked = exports.unpack(cs);
-
+  return applyZip(astr, unpacked.ops, (op1, op2) => slicerZipperFunc(op1, op2, pool));
-  return applyZip(astr, unpacked.ops,
-      (op1, op2, opOut) => slicerZipperFunc(op1, op2, opOut, pool));
 };
 
 exports.mutateAttributionLines = (cs, lines, pool) => {
@@ -1293,16 +1288,13 @@ exports.mutateAttributionLines = (cs, lines, pool) => {
 
   const isNextMutOp = () => (lineIter && lineIter.hasNext()) || mut.hasMore();
 
-  const nextMutOp = (destOp) => {
+  const nextMutOp = () => {
     if ((!(lineIter && lineIter.hasNext())) && mut.hasMore()) {
       const line = mut.removeLines(1);
       lineIter = exports.opIterator(line);
     }
-    if (lineIter && lineIter.hasNext()) {
+    if (!lineIter || !lineIter.hasNext()) return exports.newOp();
-      lineIter.next(destOp);
+    return lineIter.next();
-    } else {
-      destOp.opcode = '';
-    }
   };
   let lineAssem = null;
 
@@ -1318,13 +1310,10 @@ exports.mutateAttributionLines = (cs, lines, pool) => {
     lineAssem = null;
   };
 
-  const csOp = exports.newOp();
+  let csOp = exports.newOp();
-  const attOp = exports.newOp();
+  let attOp = exports.newOp();
-  const opOut = exports.newOp();
   while (csOp.opcode || csIter.hasNext() || attOp.opcode || isNextMutOp()) {
-    if ((!csOp.opcode) && csIter.hasNext()) {
+    if (!csOp.opcode && csIter.hasNext()) csOp = csIter.next();
-      csIter.next(csOp);
-    }
     if ((!csOp.opcode) && (!attOp.opcode) && (!lineAssem) && (!(lineIter && lineIter.hasNext()))) {
       break; // done
     } else if (csOp.opcode === '=' && csOp.lines > 0 && (!csOp.attribs) &&
@@ -1333,29 +1322,22 @@ exports.mutateAttributionLines = (cs, lines, pool) => {
       mut.skipLines(csOp.lines);
       csOp.opcode = '';
     } else if (csOp.opcode === '+') {
+      const opOut = copyOp(csOp);
       if (csOp.lines > 1) {
         const firstLineLen = csBank.indexOf('\n', csBankIndex) + 1 - csBankIndex;
-        copyOp(csOp, opOut);
         csOp.chars -= firstLineLen;
         csOp.lines--;
         opOut.lines = 1;
         opOut.chars = firstLineLen;
       } else {
-        copyOp(csOp, opOut);
         csOp.opcode = '';
       }
       outputMutOp(opOut);
       csBankIndex += opOut.chars;
-      opOut.opcode = '';
     } else {
-      if ((!attOp.opcode) && isNextMutOp()) {
+      if (!attOp.opcode && isNextMutOp()) attOp = nextMutOp();
-        nextMutOp(attOp);
+      const opOut = slicerZipperFunc(attOp, csOp, pool);
-      }
+      if (opOut.opcode) outputMutOp(opOut);
-      slicerZipperFunc(attOp, csOp, opOut, pool);
-      if (opOut.opcode) {
-        outputMutOp(opOut);
-        opOut.opcode = '';
-      }
     }
   }
 
@@ -1446,13 +1428,13 @@ exports.compose = (cs1, cs2, pool) => {
   const bankIter2 = exports.stringIterator(unpacked2.charBank);
   const bankAssem = exports.stringAssembler();
 
-  const newOps = applyZip(unpacked1.ops, unpacked2.ops, (op1, op2, opOut) => {
+  const newOps = applyZip(unpacked1.ops, unpacked2.ops, (op1, op2) => {
     const op1code = op1.opcode;
     const op2code = op2.opcode;
     if (op1code === '+' && op2code === '-') {
       bankIter1.skip(Math.min(op1.chars, op2.chars));
     }
-    slicerZipperFunc(op1, op2, opOut, pool);
+    const opOut = slicerZipperFunc(op1, op2, pool);
     if (opOut.opcode === '+') {
       if (op2code === '+') {
         bankAssem.append(bankIter2.take(opOut.chars));
@@ -1460,6 +1442,7 @@ exports.compose = (cs1, cs2, pool) => {
         bankAssem.append(bankIter1.take(opOut.chars));
       }
     }
+    return opOut;
   });
 
   return exports.pack(len1, len3, newOps, bankAssem.toString());
@@ -1788,9 +1771,8 @@ exports.copyAText = (atext1, atext2) => {
 exports.appendATextToAssembler = (atext, assem) => {
   // intentionally skips last newline char of atext
   const iter = exports.opIterator(atext.attribs);
-  const op = exports.newOp();
   while (iter.hasNext()) {
-    iter.next(op);
+    const op = iter.next();
     if (!iter.hasNext()) {
       // last op, exclude final newline
       if (op.lines <= 1) {
@@ -1994,25 +1976,19 @@ exports.makeAttribsString = (opcode, attribs, pool) => {
 exports.subattribution = (astr, start, optEnd) => {
   const iter = exports.opIterator(astr);
   const assem = exports.smartOpAssembler();
-  const attOp = exports.newOp();
+  let attOp = exports.newOp();
   const csOp = exports.newOp();
-  const opOut = exports.newOp();
 
   const doCsOp = () => {
     if (!csOp.chars) return;
     while (csOp.opcode && (attOp.opcode || iter.hasNext())) {
-      if (!attOp.opcode) iter.next(attOp);
+      if (!attOp.opcode) attOp = iter.next();
-
       if (csOp.opcode && attOp.opcode && csOp.chars >= attOp.chars &&
           attOp.lines > 0 && csOp.lines <= 0) {
         csOp.lines++;
       }
-
+      const opOut = slicerZipperFunc(attOp, csOp, null);
-      slicerZipperFunc(attOp, csOp, opOut, null);
+      if (opOut.opcode) assem.append(opOut);
-      if (opOut.opcode) {
-        assem.append(opOut);
-        opOut.opcode = '';
-      }
     }
   };
 
@@ -2025,10 +2001,7 @@ exports.subattribution = (astr, start, optEnd) => {
     if (attOp.opcode) {
       assem.append(attOp);
     }
-    while (iter.hasNext()) {
+    while (iter.hasNext()) assem.append(iter.next());
-      iter.next(attOp);
-      assem.append(attOp);
-    }
   } else {
     csOp.opcode = '=';
     csOp.chars = optEnd - start;
@@ -2067,7 +2040,7 @@ exports.inverse = (cs, lines, alines, pool) => {
   let curChar = 0;
   let curLineOpIter = null;
   let curLineOpIterLine;
-  const curLineNextOp = exports.newOp('+');
+  let curLineNextOp = exports.newOp('+');
 
   const unpacked = exports.unpack(cs);
   const csIter = exports.opIterator(unpacked.ops);
@@ -2081,7 +2054,7 @@ exports.inverse = (cs, lines, alines, pool) => {
       let indexIntoLine = 0;
       let done = false;
       while (!done && curLineOpIter.hasNext()) {
-        curLineOpIter.next(curLineNextOp);
+        curLineNextOp = curLineOpIter.next();
         if (indexIntoLine + curLineNextOp.chars >= curChar) {
           curLineNextOp.chars -= (curChar - indexIntoLine);
           done = true;
@@ -2099,9 +2072,7 @@ exports.inverse = (cs, lines, alines, pool) => {
         curLineNextOp.chars = 0;
         curLineOpIter = exports.opIterator(alinesGet(curLine));
       }
-      if (!curLineNextOp.chars) {
+      if (!curLineNextOp.chars) curLineNextOp = curLineOpIter.next();
-        curLineOpIter.next(curLineNextOp);
-      }
       const charsToUse = Math.min(numChars, curLineNextOp.chars);
       func(charsToUse, curLineNextOp.attribs, charsToUse === curLineNextOp.chars &&
           curLineNextOp.lines > 0);
@@ -2217,8 +2188,8 @@ exports.follow = (cs1, cs2, reverseInsertOrder, pool) => {
 
   const hasInsertFirst = exports.attributeTester(['insertorder', 'first'], pool);
 
-  const newOps = applyZip(unpacked1.ops, unpacked2.ops, (op1, op2, opOut) => {
+  const newOps = applyZip(unpacked1.ops, unpacked2.ops, (op1, op2) => {
-    clearOp(opOut);
+    const opOut = exports.newOp();
     if (op1.opcode === '+' || op2.opcode === '+') {
       let whichToDo;
       if (op2.opcode !== '+') {
@@ -2336,6 +2307,7 @@ exports.follow = (cs1, cs2, reverseInsertOrder, pool) => {
         newLen += opOut.chars;
         break;
     }
+    return opOut;
   });
   newLen += oldLen - oldPos;