即使是偉大的數學家也會犯錯

我們知道數學是一門精確的科學。我們對互動式數學學習軟體 GeoGebra 也能這麼說嗎？讓我們用PVS-Studio來分析一下專案源碼吧！

介紹

你還記得你在大學是如何學習電腦科學的嗎？所有這些矩陣和向量乘法、多項式方程式、插值、外推…如果我們在真實的專案中而不是在另一個實驗室報告中查看這些可怕的公式會怎麼樣？如果我們在這樣的程式碼庫中挖掘問題怎麼辦？我建議運行 PVS-Studio 並撣掉數學教科書的灰塵。為什麼是教科書？讓我告訴你。

數學挑戰

檢查此類程式原始碼的主要挑戰之一是了解正在發生的事情。在查看分析器報告時，我們對警告是否表明真正的問題有疑問。

讓我們來看看以下片段：

@Override
public void compute() {
  ....
  if (cumulative != null && cumulative.getBoolean()) {
    ....
  } else {
    ....
    branchAtoMode = fv.wrap().subtract(a).multiplyR(2)
      .divide(bEn.subtract(a).multiply(modeEn.subtract(a)));
    branchModeToB = fv.wrap().subtract(b).multiplyR(2)                
      .divide(bEn.subtract(a).multiply(modeEn.subtract(b)));
      rightBranch = new MyDouble(kernel, 0);
  }
  ....
}

我們收到以下 PVS-Studio 警告：

V6072 發現兩個相似的程式碼片段。也許，這是一個拼字錯誤，應該使用“b”變數而不是“a”。 AlgoTriangleDF.java 145、AlgoTriangleDF.java 146、AlgoTriangleDF.java 147、AlgoTriangleDF.java 148

真的是打字錯誤嗎？經過快速研究，一旦我們找到了正確的公式，我們就可以說一切都寫對了。

程式碼片段評估三角分佈，即該分佈的機率密度函數（PDF）。我們找到公式：

現在讓我們來看看程式碼。

這裡，* fv* 是一個函數變數。 wrap 回傳 wrapper，然後執行必要的數學運算。有趣的是，有 multiply 和 multiplyR 方法。在第二種方法中，R 代表 右 並交換運算元，因為乘法並不總是可交換的。

因此，第二個表達式結果寫入branchAToMode，第四個表達式寫入branchModeToB。

我們也注意到，在 branchModeToB 中，分子和分母的符號已更改。我們得到以下表達式：

表情值沒有改變。

因此，我們刷新了數學知識來理解我們收到的一些警告。識別程式碼中是否存在真正的錯誤並不難，但很難理解它應該是什麼。

錯誤

遺失代碼段

讓我們先從簡單開始，看看下面的方法：

private void updateSide(int index, int newBottomPointsLength) {
  ....
  GeoSegmentND[] s = new GeoSegmentND[4];
  GeoSegmentND segmentBottom = outputSegmentsBottom.getElement(index);
  s[0] = segmentBottom;
  s[1] = segmentSide1;
  s[2] = segmentSide2;
  s[2] = segmentSide3;
  polygon.setSegments(s);
  polygon.calcArea();
}

我們發現有人忘記將 s[2] 替換為 s[3]。最後一行的效果非常出色。這是傳說中的、非常常見的複製貼上錯誤。結果，第四個數組項遺失，並且為 null!

V6033 具有相同按鍵「2」的項目已被更改。 AlgoPolyhedronNetPrism.java 376、AlgoPolyhedronNetPrism.java 377

價值觀呢？

現在嘗試在以下程式碼片段中發現問題：

static synchronized HashMap<String, String> getGeogebraMap() {
  ....
  geogebraMap.put("−", "-");
  geogebraMap.put("⊥", "# ");
  geogebraMap.put("∼", "~ ");
  geogebraMap.put("′", "# ");
  geogebraMap.put("≤", Unicode.LESS_EQUAL + "");
  geogebraMap.put("&ge;", Unicode.GREATER_EQUAL + "");
  geogebraMap.put("&infin;", Unicode.INFINITY + "");
  ....
  geogebraMap.put("∏", "# ");
  geogebraMap.put("&Product;", "# ");
  geogebraMap.put("〉", "# ");
  geogebraMap.put("&rangle;", "# ");
  geogebraMap.put("&rarr;", "# ");
  geogebraMap.put("&rArr;", "# ");
  geogebraMap.put("&RightAngleBracket;", "# ");
  geogebraMap.put("&rightarrow;", "# ");
  geogebraMap.put("&Rightarrow;", "# ");
  geogebraMap.put("&RightArrow;", "# ");
  geogebraMap.put("&sdot;", "* ");
  geogebraMap.put("∼", "# ");
  geogebraMap.put("∝", "# ");
  geogebraMap.put("&Proportional;", "# ");
  geogebraMap.put("&propto;", "# ");
  geogebraMap.put("⊂", "# ");
  ....
  return geogebraMap;
}

多麼壯麗的景色啊！讀起來很愉快，這只是一小部分，因為這個方法從第 66 行開始，到第 404 行結束。分析儀發出 50 個 V6033 類型的警告。讓我們快速瀏覽一下其中一個警告：

V6033 已新增相同按鍵「∼」的項目。 MathMLParser.java 229, MathMLParser.java 355

讓我們刪除多餘的片段，看看涉及警告的表達式：

geogebraMap.put("∼", "~ ");
....
geogebraMap.put("∼", "# ");

不過，這很有趣。方法呼叫之間的間隔是多少？有126行。好吧，祝你好運手動發現這樣的錯誤！

大多數是鍵和值重複。然而，有一些情況與上面的範例類似，開發人員用不同的值覆蓋該值。我們應該使用哪一個？

圓形或橢圓形

@Override
protected boolean updateForItSelf() {
  ....
  if (conic.getType() == GeoConicNDConstants.CONIC_SINGLE_POINT) {
    ....
  } else {
    if (visible != Visible.FRUSTUM_INSIDE) {
      ....
      switch (conic.getType()) {
        case GeoConicNDConstants.CONIC_CIRCLE:
          updateEllipse(brush);                     // <=
          break;
        case GeoConicNDConstants.CONIC_ELLIPSE:
          updateEllipse(brush);                     // <=
          break;
        case GeoConicNDConstants.CONIC_HYPERBOLA:
          updateHyperbola(brush);
          break;
        case GeoConicNDConstants.CONIC_PARABOLA:
          updateParabola(brush);
          break;
        case GeoConicNDConstants.CONIC_DOUBLE_LINE:
          createTmpCoordsIfNeeded();
          brush.segment(tmpCoords1.setAdd3(m, tmpCoords1.setMul3(d, minmax[0])),
              tmpCoords2.setAdd3(m, tmpCoords2.setMul3(d, minmax[1])));
          break;
        case GeoConicNDConstants.CONIC_INTERSECTING_LINES:
        case GeoConicNDConstants.CONIC_PARALLEL_LINES:
          updateLines(brush);
          break;
        default:
          break;
      }
    }
  }
}

橢圓和圓都會呼叫橢圓的方法。事實上，我們可以假設這是可以的，因為圓也是橢圓。但是，該類別還有 updateCircle 方法。那應該是什麼？讓我們更深入地探討一下。

一切都發生在DrawConic3D類別。以下是橢圓和圓的方法：

protected void updateCircle(PlotterBrush brush) {
  if (visible == Visible.CENTER_OUTSIDE) {
    longitude = brush.calcArcLongitudesNeeded(e1, alpha,
          getView3D().getScale());
    brush.arc(m, ev1, ev2, e1, beta - alpha, 2 * alpha, longitude);
  } else {
    longitude = brush.calcArcLongitudesNeeded(e1, Math.PI,
          getView3D().getScale());
    brush.circle(m, ev1, ev2, e1, longitude);
  }
}
protected void updateEllipse(PlotterBrush brush) {
  if (visible == Visible.CENTER_OUTSIDE) {
    brush.arcEllipse(m, ev1, ev2, e1, e2, beta - alpha, 2 * alpha);
  } else {
    brush.arcEllipse(m, ev1, ev2, e1, e2, 0, 2 * Math.PI);
  }
}

Well... It doesn't give that much confidence. The method bodies are different, but nothing here indicates that we risk displaying unacceptable geometric objects if the method is called incorrectly.

Could there be other clues? A whole single one! The updateCircle method is never used in the project. Meanwhile, updateEllipse is used four times: twice in the first fragment and then twice in DrawConicSection3D, the inheritor class of* DrawConic3D*:

@Override
protected void updateCircle(PlotterBrush brush) {
  updateEllipse(brush);
}

@Override
protected void updateEllipse(PlotterBrush brush) {
  // ....
  } else {
    super.updateEllipse(brush);
  }
}

This updateCircle isn't used, either. So, updateEllipse only has a call in its own override and in the fragment where we first found updateForItSelf. In schematic form, the structure looks like as follows:

On the one hand, it seems that the developers wanted to use the all-purpose updateEllipse method to draw a circle. On the other hand, it's a bit strange that DrawConicSection3D has the updateCircle method that calls updateEllipse. However, updateCircle will never be called.

It's hard to guess what the fixed code may look like if the error is actually in the code. For example, if updateCircle needs to call updateEllipse in DrawConicSection3D, but DrawConic3D needs a more optimized algorithm for the circle, the fixed scheme might look like this:

So, it seems that developers once wrote updateCircle and then lost it, and we may have found its intended "home". Looks like we have discovered the ruins of the refactoring after which the developers forgot about the "homeless" method. In any case, it's worth rewriting this code to make it clearer so that we don't end up with so many questions.

All these questions have arisen because of the PVS-Studio warning. That's the warning, by the way:

V6067 Two or more case-branches perform the same actions. DrawConic3D.java 212, DrawConic3D.java 215

Order of missing object

private void updateOrdering(GeoElement geo, ObjectMovement movement) {
  ....
  switch (movement) {
    ....
    case FRONT:
      ....
      if (index == firstIndex) {
        if (index != 0) { 
          geo.setOrdering(orderingDepthMidpoint(index));
        }
        else { 
          geo.setOrdering(drawingOrder.get(index - 1).getOrdering() - 1);
        }
      }
    ....
  }
  ....
}

We get the following warning:

V6025 Index 'index - 1' is out of bounds. LayerManager.java 393

This is curious because, in the else block, the index variable is guaranteed to get the value 0. So, we pass -1 as an argument to the get method. What's the result? We catch an IndexOutOfBoundsException.

Triangles

@Override
protected int getGLType(Type type) {
  switch (type) {
  case TRIANGLE_STRIP:
    return GL.GL_TRIANGLE_STRIP;
  case TRIANGLE_FAN:
    return GL.GL_TRIANGLE_STRIP;     // <=
  case TRIANGLES:
    return GL.GL_TRIANGLES;
  case LINE_LOOP:
    return GL.GL_LINE_LOOP;
  case LINE_STRIP:
    return GL.GL_LINE_STRIP;
  }

  return 0;
}

The code is new, but the error is already well-known. It's quite obvious that GL.GL_TRIANGLE_STRIP should be GL.GL_TRIANGLE_FAN instead*.* The methods may be similar in some ways, but the results are different. You can read about it under the spoiler.

V6067 Two or more case-branches perform the same actions. RendererImplShadersD.java 243, RendererImplShadersD.java 245

To describe a series of triangles, we need to save the coordinates of the three vertices of each triangle. Thus, given N triangles, we need the saved 3N vertices. If we describe a polyhedral object using a polygon mesh, it's important to know if the triangles are connected. If they are, we can use the Triangle Strip or the Triangle Fan to describe the set of triangles using N + 2 vertices.

We note that the Triangle Fan has been removed in Direct3D 10. In OpenGL 4.6, this primitive still exists.

The Triangle Fan uses one center vertex as common, as well as the last vertex and the new vertex. Look at the following example:

To describe it, we'd need the entry (A, B, C, D, E, F, G). There are five triangles and seven vertices in the entry.

The Triangle Strip uses the last two vertices and a new one. For instance, we can create the image below using the same sequence of vertices:

Therefore, if we use the wrong primitive, we'll get dramatically different results.

Overwritten values

public static void addToJsObject(
JsPropertyMap<Object> jsMap, Map<String, Object> map) {
  for (Entry<String, Object> entry : map.entrySet()) {
    Object object = entry.getValue();
      if (object instanceof Integer) {
        jsMap.set(entry.getKey(), unbox((Integer) object));
      } if (object instanceof String[]) {                          // <=
        JsArray<String> clean = JsArray.of();
        for (String s: (String[]) object) {
          clean.push(s);
        }
        jsMap.set(entry.getKey(), clean);
    } else {                                                       // <=
      jsMap.set(entry.getKey(), object);                           // <=
    }
  }
}

If we don't look closely, we may not notice the issue right away. However, let's speed up and just check the analyzer message.

V6086 Suspicious code formatting. 'else' keyword is probably missing. ScriptManagerW.java 209

Finally, a more interesting bug is here. The object instanceof String[] check will occur regardless of the result of object instanceof Integer. It may not be a big deal, but there's also the else block that will be executed after a failed check for String[]. As the result, the jsMap value by entry.getKey() will be overwritten if the object was Integer.

There's another similar case, but the potential consequences are a little harder to understand:

public void bkspCharacter(EditorState editorState) {
  int currentOffset = editorState.getCurrentOffsetOrSelection();
  if (currentOffset > 0) {
    MathComponent prev = editorState.getCurrentField()
        .getArgument(currentOffset - 1);
    if (prev instanceof MathArray) {
      MathArray parent = (MathArray) prev;
      extendBrackets(parent, editorState);
    } if (prev instanceof MathFunction) {                          // <=
      bkspLastFunctionArg((MathFunction) prev, editorState);
    } else {                                                       // <=
      deleteSingleArg(editorState);
    }
  } else {
    RemoveContainer.withBackspace(editorState);
  }
}

V6086 Suspicious code formatting. 'else' keyword is probably missing. InputController.java 854

Almost correct

Do you often have to write many repetitious checks? Are these checks prone to typos? Let's look at the following method:

public boolean contains(BoundingBox other) {
  return !(isNull() || other.isNull()) && other.minx >= minx
      && other.maxy <= maxx && other.miny >= miny
      && other.maxy <= maxy;
}

V6045 Possible misprint in expression 'other.maxy <= maxx'. BoundingBox.java 139

We find a typo, we fix it. The answer is simple, there should be other.maxx <= maxx.

Mixed up numbers

public PointDt[] calcVoronoiCell(TriangleDt triangle, PointDt p) {
  ....
  // find the neighbor triangle
  if (!halfplane.next_12().isHalfplane()) {
    neighbor = halfplane.next_12();
  } else if (!halfplane.next_23().isHalfplane()) {
    neighbor = halfplane.next_23();
  } else if (!halfplane.next_23().isHalfplane()) { // <=
    neighbor = halfplane.next_31();
  } else {
    Log.error("problem in Delaunay_Triangulation");
    // TODO fix added by GeoGebra
    // should we do something else?
    return null;
  }
  ....
}

Let's see the warning:

V6003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. DelaunayTriangulation.java 678, DelaunayTriangulation.java 680

We don't even need to figure out what happens to half-planes, because it's clear that the !halfplane.next_31().isHalfplane() check is missing.

Wrong operation

public static ExpressionNode get(
    ExpressionValue left, ExpressionValue right, 
    Operation operation, FunctionVariable fv, 
    Kernel kernel0
) {
  ....
  switch (operation) {
    ....
    case VEC_FUNCTION:
      break;
    case ZETA:
      break;
    case DIRAC:
      return new ExpressionNode(kernel0, left, Operation.DIRAC, fv);
    case HEAVISIDE:
      return new ExpressionNode(kernel0, left, Operation.DIRAC, fv);
    default:
      break;
  }
  ....
}

It seems that, in the second case, Operation.DIRAC should probably be replaced with Operation.HEAVISIDE. Or not? This method is called to obtain the derivative. After researching, we understand what HEAVISIDE *is—the use of *Operation.DIRAC for it is correct. What about the Dirac derivative? This one is a bit tricky to understand. We may trust the developers and suppress the warning. Still, it'd be better if they'd left any explanatory comment as they had done in some cases before.

case FACTORIAL:
  // x! -> psi(x+1) * x!
  return new ExpressionNode(kernel0, left.wrap().plus(1),
      Operation.PSI, null)
          .multiply(new ExpressionNode(kernel0, left,
              Operation.FACTORIAL, null))
          .multiply(left.derivative(fv, kernel0));

case GAMMA:
  // gamma(x) -> gamma(x) psi(x)
  return new ExpressionNode(kernel0, left, Operation.PSI, null)
      .multiply(new ExpressionNode(kernel0, left, Operation.GAMMA,
          null))
      .multiply(left.derivative(fv, kernel0));

And we were motivated to conduct such research by the message of already favorite diagnostic V6067:

V6067 Two or more case-branches perform the same actions. Derivative.java 442, Derivative.java 444

On the one hand, this is an interesting case because we could have a false positive here. On the other, the warning would be useful either way, as it often highlights a genuine error. Regardless of the results, we need to check such code and either fix the error or write explanatory comments. Then the warning can be suppressed.

Vector or point?

private static GeoElement doGetTemplate(Construction cons,
    GeoClass listElement) {
  switch (listElement) {
  case POINT:
    return new GeoPoint(cons);
  case POINT3D:
    return (GeoElement) cons.getKernel().getGeoFactory().newPoint(3, cons);
  case VECTOR:
    return new GeoVector(cons);
  case VECTOR3D:
    return (GeoElement) cons.getKernel().getGeoFactory().newPoint(3, cons);
  }
  return new GeoPoint(cons);
}

The warning isn't hard to guess:

V6067 Two or more case-branches perform the same actions. PointNDFold.java 38, PointNDFold.java 43

Instead of cons.getKernel().getGeoFactory().newPoint(3, cons) in the second case, cons.getKernel().getGeoFactory().newVector(3, cons) may have been intended. If we want to make sure of it, we need to go deeper again. Well, let's dive into it.

So,* getGeoFactory() returns GeoFactory, let's look at the *newPoint *and newVector* methods:

public GeoPointND newPoint(int dimension, Construction cons) {
  return new GeoPoint(cons);
}
public GeoVectorND newVector(int dimension, Construction cons) {
  return new GeoVector(cons);
}

It looks a bit strange. The dimension argument isn't used at all. What's going on here? Inheritance, of course! Let's find the GeoFactory3D inheritor class and see what happens in it.

@Override
public GeoVectorND newVector(int dimension, Construction cons) {
  if (dimension == 3) {
    return new GeoVector3D(cons);
  }
  return new GeoVector(cons);
}
@Override
public GeoPointND newPoint(int dimension, Construction cons) {
  return dimension == 3 ? new GeoPoint3D(cons) : new GeoPoint(cons);
}

Excellent! Now we can admire the creation of four different objects in all their glory. We return to the code fragment with the possible error. For POINT and POINT3D, the objects of the GeoPoint and GeoPoint3D classes are created. GeoVector is created for VECTOR, but poor GeoVector3D seems to have been abandoned.

Sure, it's a bit strange to use a factory method pattern in two cases and call constructors directly in the remaining two. Is this a leftover from the refactoring process, or is it some kind of temporary solution that'll stick around until the end of time? In my opinion, if the responsibility for creating objects has been handed over to another class, it'd be better to fully delegate that responsibility.

Missing quadrillions

@Override
final public void update() {
  ....
  switch (angle.getAngleStyle()) {
    ....
    case EuclidianStyleConstants.RIGHT_ANGLE_STYLE_L:
      // Belgian offset |_
      if (square == null) {
        square = AwtFactory.getPrototype().newGeneralPath();
      } else {
        square.reset();
      }
      length = arcSize * 0.7071067811865;                   // <=
      double offset = length * 0.4;
      square.moveTo(
          coords[0] + length * Math.cos(angSt)
              + offset * Math.cos(angSt)
              + offset * Math.cos(angSt + Kernel.PI_HALF),
          coords[1]
              - length * Math.sin(angSt)
                  * view.getScaleRatio()
              - offset * Math.sin(angSt)
              - offset * Math.sin(angSt + Kernel.PI_HALF));
      ....
      break;
    }
}

Where's the warning? There it is:

V6107 The constant 0.7071067811865 is being utilized. The resulting value could be inaccurate. Consider using Math.sqrt(0.5). DrawAngle.java 303

我們發現了此程式碼片段的四個實例。更準確的值為0.7071067811865476。挑戰自己，試著背熟它，呵呵。最後三位數字被省略。這很關鍵嗎？準確性可能足夠了，但使用預先定義的常數或 Math.sqrt(0.5)（如本例所示）不僅有助於恢復丟失的數字，還可以消除拼寫錯誤的風險。請注意程式碼可讀性將如何增強。也許當看到0.707…有人立刻明白這是一個什麼樣的神奇數字。然而，有時我們可以做一些不那麼神奇的事情來增強程式碼的可讀性。

結論

我們小小的數學之旅結束了。正如我們所看到的，即使在解決了許多幾何挑戰之後，人們仍然可能在程式碼中犯一些簡單的錯誤！如果程式碼是新鮮的，那麼開發人員仍然可以牢記他們過去的意圖，那就太好了。在這種情況下，人們可能會明白需要做什麼改變（如果有的話），但時間長了之後可能就不那麼容易認識到解決這些問題的必要性了。

因此，事實證明，分析器在編寫程式碼時非常高效，而不是像我那樣運行一次。尤其是現在，當我們擁有像 PVS-Studio 分析儀這樣的工具時。

我們關於類似主題的其他文章

1. 使用數學軟體很頭痛。
2. 數學家：信任，但要驗證。
3. 瘋狂的大計算器。
4. 跨蛋白質體管道（TPP）項目分析。
5. COVID-19 研究和未初始化變數。
6. 分析ROOT的程式碼，科學的資料分析架構。
7. NCBI 基因組工作台：受到威脅的科學研究。

以上是即使是偉大的數學家也會犯錯的詳細內容。更多資訊請關注PHP中文網其他相關文章！