FreeCADのソースコードとその「悪い」依存関係の確認

この記事は、オープンなFreeCADプロジェクトのエラーの概要を目的としており、わずかに異なる性格を獲得しました。 アナライザーの警告の大部分は、使用されているサードパーティのライブラリから発生しました。 サードパーティライブラリを積極的に使用したソフトウェア開発には、特にオープンソースの分野で多くの利点があります。 そして、ライブラリのエラーはそれらを拒否する理由ではありません。 ただし、使用するコードにもバグが存在する可能性があることを理解する必要があります。 彼らは会う準備ができていて、場合によっては修正する必要があり、それによってあなたが使用するライブラリを改善します。



FreeCADは、3次元モデルとその投影図を作成できるパラメトリック3次元エディターです。 DaimlerChrysler Corporationに勤務するFreeCAD開発者JürgenRigelは、彼のプログラムを最初の無料の機械設計ツールとして位置付けています。 多くの業界の専門家の間では、オープンソースのフレームワーク内で本格的なCADシステムを作成する問題が知られており、このプロジェクトはそのような「完全な価値」の候補です。 PVS-Studioを使用してソースコードを確認し、この分野のオープンソースプロジェクトが少し良くなるようにします。 常に1ピクセル移動するポイントに到達したり、直線をまっすぐにしたりできないときに、さまざまなエディターで「グリッチ」に遭遇しました。 おそらく、これらすべての理由は、ソースコードのタイプミスにすぎません。

PVS-Studioの何が問題になっていますか？！

FreeCADプロジェクトはクロスプラットフォームであり、サイトには非常に優れたアセンブリドキュメントがあります。 インストールされたPVS-Studioプラグインを使用して、検証のためにVisual Studio Community 2013のプロジェクトファイルを取得することは難しくありませんでした。 しかし、最初はチェックがうまくいきませんでした...











アナライザーの内部エラーの原因は、* .i拡張子を持つテキスト前処理ファイルにバイナリシーケンスが存在することでした。 アナライザーはそのような状況を処理できますが、何か新しいことが起こりました。 問題は、ソースファイルのコンパイルオプションのいずれかの行にあります。

/FI"Drawing.dir/Debug//Drawing_d.pch"
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

コンパイルフラグ/ FI（名前強制インクルードファイル）と#includeディレクティブは、テキストヘッダーファイルを含めるために使用されます。 しかし、ここでは、バイナリ形式の情報を含むファイルを含めようとしています。 奇跡的にコンパイルします。 おそらくVisual C ++では、このようなファイルは単に無視されます。



コンパイルしない、つまりファイルを前処理しようとすると、Visual C ++はエラーを報告します。 そして、ここでは、PVS-Studioでデフォルトで使用されているClangには、考え直さずに* .iファイルとバイナリファイルが含まれていました。 PVS-Studioはそのようなトリックを期待していなかったため、夢中になりました。



危機にwhatしているものを明確にするために、Clangで前処理されたファイルのフラグメントを次に示します。











このフラグを使用せずにプロジェクトを慎重にチェックしましたが、ここに間違いがあるという開発者の注意を引きたいと思います。

Freecad

プロジェクトのエラーの最初の例は、誰もがよく知っている理由で得られました。







V501 「！=」演算子の左右には、同一の副次式「surfaceTwo-> IsVRational（）」があります。 modelrefine.cpp 780

 bool FaceTypedBSpline::isEqual(const TopoDS_Face &faceOne, const TopoDS_Face &faceTwo) const { .... if (surfaceOne->IsURational() != surfaceTwo->IsURational()) return false; if (surfaceTwo->IsVRational() != surfaceTwo->IsVRational())//<= return false; if (surfaceOne->IsUPeriodic() != surfaceTwo->IsUPeriodic()) return false; if (surfaceOne->IsVPeriodic() != surfaceTwo->IsVPeriodic()) return false; if (surfaceOne->IsUClosed() != surfaceTwo->IsUClosed()) return false; if (surfaceOne->IsVClosed() != surfaceTwo->IsVClosed()) return false; if (surfaceOne->UDegree() != surfaceTwo->UDegree()) return false; if (surfaceOne->VDegree() != surfaceTwo->VDegree()) return false; .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

不等式演算子の左側で、タイプミスが原因で、「surfaceOne」の代わりに間違った変数「surfaceTwo」が見つかりました。 次回、コピーペーストフラグメントを大きくするために著者にアドバイスすることは残っていますが、このような例についても説明します=）。



V517 「if（A）{...} else if（A）{...}」パターンの使用が検出されました。 論理エラーが存在する可能性があります。 行を確認：162、164。taskpanelview.cpp 162

 /// @cond DOXERR void TaskPanelView::OnChange(....) { std::string temp; if (Reason.Type == SelectionChanges::AddSelection) { } else if (Reason.Type == SelectionChanges::ClrSelection) { } else if (Reason.Type == SelectionChanges::RmvSelection) { } else if (Reason.Type == SelectionChanges::RmvSelection) { } }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

まだ書かれている機能に注意を払ったのはなぜですか？ その理由は次のとおりです。このコードを使用すると、次の2つの例の場合とほとんど同じになります。



V517 「if（A）{...} else if（A）{...}」パターンの使用が検出されました。 論理エラーが存在する可能性があります。 行を確認してください：1465、1467。application.cpp 1465

 pair<string, string> customSyntax(const string& s) { #if defined(FC_OS_MACOSX) if (s.find("-psn_") == 0) return make_pair(string("psn"), s.substr(5)); #endif if (s.find("-display") == 0) return make_pair(string("display"), string("null")); else if (s.find("-style") == 0) return make_pair(string("style"), string("null")); .... else if (s.find("-button") == 0) //<== return make_pair(string("button"), string("null")); //<== else if (s.find("-button") == 0) //<== return make_pair(string("button"), string("null")); //<== else if (s.find("-btn") == 0) return make_pair(string("btn"), string("null")); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

著者がコピーした1行を誤って修正しなかったことを願っていますが、結局、必要なすべての行の検索を追加しました。



V517 「if（A）{...} else if（A）{...}」パターンの使用が検出されました。 論理エラーが存在する可能性があります。 行を確認：191、199。blendernavigationstyle.cpp 191

 SbBool BlenderNavigationStyle::processSoEvent(....) { .... else if (!press && (this->currentmode == NavigationStyle::DRAGGING)) { //<== SbTime tmp = (ev->getTime() - this->centerTime); float dci = (float)QApplication::....; if (tmp.getValue() < dci) { newmode = NavigationStyle::ZOOMING; } processed = TRUE; } else if (!press && (this->currentmode == NavigationStyle::DRAGGING)) { //<== this->setViewing(false); processed = TRUE; } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

しかし、私の意見では、そのようなアプリケーションの重大な間違いです。 モデリング時には、マウスナビゲーションを使用して多くの作業が行われますが、ここにそのような間違いがあります。最初の条件が同じで最初の条件が実行されるため、最後の条件のソースコードは制御されません。



V523 「then」ステートメントは「else」ステートメントと同等です。 viewproviderfemmesh.cpp 695

 inline void insEdgeVec(std::map<int,std::set<int> > &map, int n1, int n2) { if(n1<n2) map[n2].insert(n1); else map[n2].insert(n1); };
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

条件に関係なく、1つのアクションが常に実行されます。 多分それはまだこのように考えられていた：

 inline void insEdgeVec(std::map<int,std::set<int> > &map, int n1, int n2) { if(n1<n2) map[n2].insert(n1); else map[n1].insert(n2); };
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

最後の行を修正したのはなぜですか？ このトピックに関する記事に興味があるかもしれません： 最後の行の効果 。 しかし、おそらく最初の問題を解決する必要があります。 知りません:)。



V570 「this-> quat [3]」変数はそれ自体に割り当てられます。 rotation.cpp 260

 Rotation & Rotation::invert(void) { this->quat[0] = -this->quat[0]; this->quat[1] = -this->quat[1]; this->quat[2] = -this->quat[2]; this->quat[3] = this->quat[3]; //<== return *this; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

「最後の行」についての詳細。 アナライザーは、最後の行にマイナス記号がないため、注意を払っています。 しかし、ここでは間違いなくエラーを話すことは不可能です。おそらく、そのような変換を実装するとき、彼らは4番目のコンポーネントが変わらないことを強調したかったのです。



V576形式が正しくありません 。 'fprintf'関数を呼び出すときに、異なる数の実引数が予期されます。 予想：2.現在：3. memdebug.cpp 222

 int __cdecl MemDebug::sAllocHook(....) { .... if ( pvData != NULL ) fprintf( logFile, " at %p\n", pvData ); else fprintf( logFile, "\n", pvData ); //<== .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

このようなコードは意味がありません。 ポインターがヌルの場合、未使用のパラメーターを関数に渡すことなく、単に改行を印刷できます。



V596オブジェクトは作成されましたが、使用されていません。 「throw」キーワードが欠落している可能性があります：throw Exception（FOO）; waypointpyimp.cpp 231

 void WaypointPy::setTool(Py::Int arg) { if((int)arg.operator long() > 0) getWaypointPtr()->Tool = (int)arg.operator long(); else Base::Exception("negativ tool not allowed!"); }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

例外タイプのオブジェクトはコード内に作成されますが、使用されません。 どうやら「スロー」キーワードが欠落しているようです：

 void WaypointPy::setTool(Py::Int arg) { if((int)arg.operator long() > 0) getWaypointPtr()->Tool = (int)arg.operator long(); else throw Base::Exception("negativ tool not allowed!"); }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

さらにいくつかのそのような場所：

• V596オブジェクトは作成されましたが、使用されていません。 「throw」キーワードが欠落している可能性があります：throw Exception（FOO）; application.cpp 274
• V596オブジェクトは作成されましたが、使用されていません。 「throw」キーワードが欠落している可能性があります：throw Exception（FOO）; fileinfo.cpp 519
• V596オブジェクトは作成されましたが、使用されていません。 「throw」キーワードが欠落している可能性があります：throw Exception（FOO）; waypointpyimp.cpp 244
• V596オブジェクトは作成されましたが、使用されていません。 「throw」キーワードが欠落している可能性があります：throw Exception（FOO）; sketch.cpp 185

V599 「カーブ」クラスには仮想関数が含まれていますが、仮想デストラクタは存在しません。 constraints.cpp 1442

 class Curve { //a base class for all curve-based //objects (line, circle/arc, ellipse/arc) //<== public: virtual DeriVector2 CalculateNormal(....) = 0; virtual int PushOwnParams(VEC_pD &pvec) = 0; virtual void ReconstructOnNewPvec (....) = 0; virtual Curve* Copy() = 0; }; class Line: public Curve //<== { public: Line(){} Point p1; Point p2; DeriVector2 CalculateNormal(Point &p, double* derivparam = 0); virtual int PushOwnParams(VEC_pD &pvec); virtual void ReconstructOnNewPvec (VEC_pD &pvec, int &cnt); virtual Line* Copy(); };
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

使用法：

 class ConstraintAngleViaPoint : public Constraint { private: inline double* angle() { return pvec[0]; }; Curve* crv1; //<== Curve* crv2; //<== .... }; ConstraintAngleViaPoint::~ConstraintAngleViaPoint() { delete crv1; crv1 = 0; //<== delete crv2; crv2 = 0; //<== }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

基本クラスでは「曲線」仮想関数が宣言されていますが、デフォルトで作成されるデストラクタは宣言されていません。 そしてもちろん、仮想ではありません！ つまり、このようなユースケースでは、子クラスへのポインターが基本クラスへのポインターに格納されている場合、このクラスから継承するオブジェクトは完全にはクリーンアップされません。 コメントから判断すると、基本クラスには多くの継承クラスがあります。たとえば、この例の「Line」クラスなどです。



V655ストリングは連結されましたが、使用されません。 式を調べることを検討してください。 propertyitem.cpp 1013

 void PropertyVectorDistanceItem::setValue(const QVariant& variant) { if (!variant.canConvert<Base::Vector3d>()) return; const Base::Vector3d& value = variant.value<Base::Vector3d>(); Base::Quantity q = Base::Quantity(value.x, Base::Unit::Length); QString unit = QString::fromLatin1("('%1 %2'").arg(....; q = Base::Quantity(value.y, Base::Unit::Length); unit + QString::fromLatin1("'%1 %2'").arg(....; //<== setPropertyValue(unit); }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

アナライザーは、無意味な行の追加を検出しました。 よく見ると、おそらく単純な加算の代わりに「+ =」演算子を使用したかったでしょう。 そのようなコードは理にかなっています。



V595 nullptrに対して検証される前に、「ルート」ポインターが使用されました。 行を確認：293、294。view3dinventorexamples.cpp 293

 void LightManip(SoSeparator * root) { SoInput in; in.setBuffer((void *)scenegraph, std::strlen(scenegraph)); SoSeparator * _root = SoDB::readAll( &in ); root->addChild(_root); //<== if ( root == NULL ) return; //<== root->ref(); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

間違った場所でポインターをチェックする1つの例、他の場所はこちらです：

• V595 nullptrに対して検証される前に、「cam」ポインターが使用されました。 行を確認してください：1049、1056。viewprovider.cpp 1049
• V595 nullptrに対して検証される前に、「viewProviderRoot」ポインターが使用されました。 行を確認：187、188。taskcheckgeometry.cpp 187
• V595 nullptrに対して検証される前に、「ノード」ポインターが使用されました。 行を確認してください：209、210。viewproviderrobotobject.cpp 209
• V595 nullptrに対して検証される前に、「ノード」ポインターが使用されました。 行を確認してください：222、223。viewproviderrobotobject.cpp 222
• V595 nullptrに対して検証される前に、「ノード」ポインターが使用されました。 行を確認してください：235、236。viewproviderrobotobject.cpp 235
• V595 nullptrに対して検証される前に、「ノード」ポインターが使用されました。 行を確認してください：248、249。viewproviderrobotobject.cpp 248
• V595 nullptrに対して検証される前に、「ノード」ポインターが使用されました。 チェック行：261、262。viewproviderrobotobject.cpp 261
• V595 nullptrに対して検証される前に、「ノード」ポインターが使用されました。 行を確認してください：274、275。viewproviderrobotobject.cpp 274
• V595 nullptrに対して検証される前に、「所有者」ポインターが使用されました。 行を確認：991、995。propertysheet.cpp 991

オープンカスケードライブラリ

V519 「myIndex [1]」変数には、値が2回連続して割り当てられます。 おそらくこれは間違いです。 行を確認してください：60、61。brepmesh_pairofindex.hxx 61

 //! Prepends index to the pair. inline void Prepend(const Standard_Integer theIndex) { if (myIndex[1] >= 0) Standard_OutOfRange::Raise ("BRepMesh_PairOfIndex...."); myIndex[1] = myIndex[0]; myIndex[1] = theIndex; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

この例では、インデックス1の配列要素 'myIndex'の値が上書きされました。

 myIndex[1] = myIndex[0]; myIndex[0] = theIndex;
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

SALOME Smeshモジュール

V501 「&&」演算子の左右には、同一のサブ式「0 <= theParamsHint.Y（）」があります。 smesh_block.cpp 661

 bool SMESH_Block::ComputeParameters(const gp_Pnt& thePoint, gp_XYZ& theParams, const int theShapeID, const gp_XYZ& theParamsHint) { .... bool hasHint = ( 0 <= theParamsHint.X() && theParamsHint.X() <= 1 && 0 <= theParamsHint.Y() && theParamsHint.Y() <= 1 && 0 <= theParamsHint.Y() && theParamsHint.Y() <= 1 ); //<== .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

十分な検証.Z（）がないことは明らかです。 このクラスにはこのような機能があり、「gp_XYZ」とも呼ばれます。



V503これは無意味な比較です：ポインター<0。driverdat_r_smds_mesh.cpp 55

 Driver_Mesh::Status DriverDAT_R_SMDS_Mesh::Perform() { .... FILE* aFileId = fopen(file2Read, "r"); if (aFileId < 0) { fprintf(stderr, "....", file2Read); return DRS_FAIL; } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

ポインターをゼロより小さくすることはできません。 本やインターネットにあるfopen（）関数を使用した最も単純な例でも、関数の値は==または！=でNULLと比較されます。



そのようなコードがどのように表示されるのか疑問に思いました。 しかし、私の同僚のAndrei Karpovは、 open（）関数がかつて使用されていたコードをリファクタリングするときにこれが頻繁に起こることを示唆しました。 この関数は、ケースで-1を返し、比較<0は非常に適切です。 プログラムのリファクタリングまたは移植の過程で、この関数はfopen（）に置き換えられますが、チェックの修正を忘れています。



別のそのような場所：

• V503これは無意味な比較です：ポインター<0。driverdat_w_smds_mesh.cpp 41

V562ブール型の値を12 :! MyType == SMESHDS_MoveNodeの値と比較するのは奇妙です。 smeshds_command.cpp 75

 class SMESHDS_EXPORT SMESHDS_Command { .... private: SMESHDS_CommandType myType; .... }; enum SMESHDS_CommandType { SMESHDS_AddNode, SMESHDS_AddEdge, SMESHDS_AddTriangle, SMESHDS_AddQuadrangle, .... }; void SMESHDS_Command::MoveNode(....) { if (!myType == SMESHDS_MoveNode) //<== { MESSAGE("SMESHDS_Command::MoveNode : Bad Type"); return; } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

「SMESHDS_CommandType」という名前の列挙があり、多くの定数があります。 アナライザーが誤ったチェックを検出しました。このタイプの変数は名前付き定数と比較されますが、ここで否定記号は何をしますか?? ほとんどの場合、チェックは次のようになります。

 if (myType != SMESHDS_MoveNode) //<== { MESSAGE("SMESHDS_Command::MoveNode : Bad Type"); return; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

残念ながら、このようなエラーメッセージを含むチェックは別の20箇所にコピーされました 。完全なリストは次のとおりです： FreeCAD_V562.txt



V567未定義の動作。 引数評価の順序は、「スプライス」関数に対して定義されていません。 'outerBndPos'変数は、シーケンスポイント間で2回使用されている間に変更されます。 smesh_pattern.cpp 4260

 void SMESH_Pattern::arrangeBoundaries (....) { .... if ( outerBndPos != boundaryList.begin() ) boundaryList.splice( boundaryList.begin(), boundaryList, outerBndPos, //<== ++outerBndPos ); //<== }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

実際、アナライザーはここにはまったくありません。 未定義の動作はここにはありません。 しかし、エラーがあるため、警告は無駄ではありませんでした。 C ++標準は、実際の関数引数が評価される順序に制限を課しません。 したがって、どの値が関数に渡されるかはわかりません。



簡単な例で説明します。

 int a = 5; printf("%i, %i", a, ++a);
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

このコードは、「5、6」と「6、6」の両方を出力できます。結果は、コンパイラとその設定によって異なります。



V663無限ループが可能です。 「cin.eof（）」条件は、ループから抜けるには不十分です。 「cin.fail（）」関数呼び出しを条件式に追加することを検討してください。 unv_utilities.hxx 63

 inline bool beginning_of_dataset(....) { .... while( ((olds != "-1") || (news == "-1") ) && !in_file.eof() ){ olds = news; in_file >> news; } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

「std :: istream」クラスを使用する場合、「eof（）」関数を呼び出すだけではループを完了できません。 データの読み取り中に障害が発生した場合、「eof（）」関数を呼び出すと、常に値「false」が返されます。 この場合にループを完了するには、関数 'fail（）'によって返された値の追加チェックが必要です。



V595 nullptrに対して検証される前に、「anElem」ポインターが使用されました。 行を確認してください：1950、1951。smesh_controls.cpp 1950

 bool ElemGeomType::IsSatisfy( long theId ) { if (!myMesh) return false; const SMDS_MeshElement* anElem = myMesh->FindElement( theId ); const SMDSAbs_ElementType anElemType = anElem->GetType(); if (!anElem || (myType != SMDSAbs_All && anElemType != myType)) return false; const int aNbNode = anElem->NbNodes(); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

ポインタ「anElem」は、有効性を確認するよりも1行上で参照解除されます。



このプロジェクトの類似した場所：

• V595 nullptrに対して検証される前に、「elem」ポインターが使用されました。 行を確認：3989、3990。smesh_mesheditor.cpp 3989
• V595 nullptrに対して検証される前に、「anOldGrp」ポインターが使用されました。 行を確認してください：1488、1489。smesh_mesh.cpp 1488
• V595 nullptrに対して検証される前に、「aFaceSubmesh」ポインターが使用されました。 行をチェック：496、501。smesh_pattern.cpp 496

Boost C ++ライブラリ

V567未定義の動作。 'this-> n_'変数は、シーケンスポイント間で2回使用されている間に変更されます。 regex_token_iterator.hpp 63

 template<typename BidiIter> struct regex_token_iterator_impl : counted_base<regex_token_iterator_impl<BidiIter> > { .... if(0 != (++this->n_ %= (int)this->subs_.size()) || .... { .... } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

％=演算子のどのオペランドが最初に評価されるかは不明です。 したがって、式が正しく機能するかどうかは、運次第です。

おわりに

静的アナライザーを試して実装し、プロジェクトと定期的に使用するサードパーティライブラリを定期的にチェックしてください。 これにより、新しいコードを作成するときと、古いコードをサポートするときの時間が節約されます。





英語を話す聴衆とこの記事を共有したい場合は、翻訳へのリンクを使用してください：Svyatoslav Razmyslov。 FreeCADのソースコードとその「病気」の依存関係の分析 。