위대한 수학자도 실수를 한다-java지도 시간-php.cn

우리는 수학이 정밀성의 과학이라는 것을 알고 있습니다. 대화형 수학 학습 소프트웨어인 GeoGebra에 대해서도 똑같이 말할 수 있습니까? PVS-Studio를 이용하여 프로젝트 소스코드를 분석해보겠습니다!

Even great mathematicians make mistakes

소개

대학에서 컴퓨터공학을 어떻게 배웠는지 기억하시나요? 이 모든 행렬 및 벡터 곱셈, 다항 방정식, 보간, 외삽... 다른 실험실 보고서가 아닌 실제 프로젝트에서 이 무서운 공식을 보면 어떻게 될까요? 그러한 코드 기반에서 문제를 파헤쳐 보면 어떻게 될까요? 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 두 개의 유사한 코드 조각이 발견되었습니다. 아마도 이것은 오타이므로 'a' 대신 'b' 변수를 사용해야 합니다. AlgoTriangularDF.java 145, AlgoTriangularDF.java 146, AlgoTriangularDF.java 147, AlgoTriangularDF.java 148

정말 오타인가요? 빠르게 조사한 후 올바른 공식을 찾으면 모든 것이 올바르게 작성되었다고 말할 수 있습니다.

코드 조각은 삼각 분포, 즉 이 분포에 대한 확률 밀도 함수(PDF)를 평가합니다. 공식을 찾았습니다:

Even great mathematicians make mistakes

이제 코드를 살펴보겠습니다.

여기서* fv*는 함수 변수입니다. wrap은 wrapper를 반환한 다음 필요한 수학적 연산을 수행합니다. multiply 및 multiplyR 메서드가 모두 있다는 점이 흥미롭습니다. 두 번째 방법에서 R은 right를 나타내며 곱셈이 항상 교환 가능한 것은 아니기 때문에 피연산자를 바꿉니다.

그래서 두 번째 표현식 결과는 branchAToMode에 기록되고, 네 번째 표현식은 branchModeToB에 기록됩니다.

또한 branchModeToB에서 분자와 분모의 기호가 변경된 것을 확인했습니다. 우리는 다음과 같은 표현을 얻습니다:

Even great mathematicians make mistakes

표현값은 변하지 않았습니다.

그래서 우리는 받은 경고 중 일부를 이해하기 위해 수학적 지식을 새롭게 했습니다. 코드에 실제 오류가 있는지 식별하는 것은 어렵지 않지만, 대신 여기에 무엇이 있어야 하는지 이해하기는 어렵습니다.

오류

분실된 코드 세그먼트

간단하게 시작하여 다음 방법을 살펴보겠습니다.

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번째 줄에서 끝나기 때문에 이것은 작은 부분일 뿐입니다. 분석기는 V6033 유형의 경고 50개를 발행합니다. 다음 경고 중 하나를 간단히 살펴보겠습니다.

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:

Even great mathematicians make mistakes

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:

Even great mathematicians make mistakes

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:

Even great mathematicians make mistakes

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:

Even great mathematicians make mistakes

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

우리는 이 코드 조각의 인스턴스 4개를 발견했습니다. 더 정확한 값은 0.7071067811865476입니다. 도전해서 암기해 보세요, 헤헤. 마지막 세 자리는 생략됩니다. 중요한가요? 정확도는 충분할 것입니다. 그러나 이 경우와 같이 사전 정의된 상수 또는 Math.sqrt(0.5)를 사용하면 잃어버린 숫자를 복구하는 데 도움이 될 뿐만 아니라 오타의 위험도 제거됩니다. 코드 가독성이 어떻게 향상되는지 확인하십시오. 아마도 0.707을 보면... 누군가는 그것이 어떤 마법의 숫자인지 즉시 이해하게 될 것입니다. 그러나 때로는 코드 가독성을 높이기 위해 약간의 마법을 사용할 수도 있습니다.

결론

우리의 작은 수학 여행이 끝났습니다. 보시다시피, 많은 기하학적 문제를 해결한 후에도 사람들은 여전히 코드에서 단순한 실수를 할 수 있습니다! 코드가 최신이고 개발자가 과거 의도를 계속 염두에 둘 수 있다면 좋습니다. 이러한 경우 사람들은 어떤 변화가 필요한지 이해할 수 있지만 시간이 지나면 이러한 문제를 해결해야 할 필요성을 인식하기가 쉽지 않을 수 있습니다.

그래서 분석기는 저처럼 한 번만 실행하는 것보다 코드를 작성할 때 꽤 효율적인 것으로 입증되었습니다. 특히 PVS-Studio 분석기와 같은 도구를 자유롭게 사용할 수 있는 경우에는 더욱 그렇습니다.