View Issue Details

Related ChangesetsJump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0030448CommunityOCCT:Codingpublic2019-01-18 16:432019-08-27 14:39
ReporterBenjaminBihler Assigned Toapn  
PrioritynormalSeverityminor 
Status closedResolutionfixed 
PlatformAOSL 
Product Version7.3.0 
Target Version7.4.0Fixed in Version7.4.0 
Summary0030448: Coding - add typo detection to derivation creation methods using Standard_NODISCARD attribute
DescriptionThere are a lot of methods for modifying an object OR creating a derivation of an already existing object (especially in gp) and they all have the same naming pattern.

For example gp_Vec has method Reversed(). Since this method is pure (it is const and has no side-effects), its return value should never be discarded.

Because of the naming pattern the modification method's name is VERY similar to the derivation creation method's name (Reverse() and Reversed()). If someone wants to revert a vector and mistakenly calls vector.Reversed() instead of vector.Reverse(), he has introduced a subtle bug. A reverted vector is returned, but then discarded and the original vector remains unaltered.

From C++ 17 on there is the possibility to add a [[nodiscard]] attribute to a return value to flag that it should be used. If the Reversed() method would have been flagged like that, the error could easily be detected.

I have suffered from such an error, therefore I want to ask whether this change is welcome? It has a disadvantage: for preventing compilation errors and warnings from older compilers, the attribute needs to be surrounded by lengthy code. It would look like this:

  //! Reverses the direction of a vector
#ifdef __has_cpp_attribute
#if __has_cpp_attribute(nodiscard)
    [[nodiscard]]
#endif
#endif
    gp_Vec Reversed() const;
  gp_Vec operator -() const
{
  return Reversed();
}

What is your opinion?
Steps To ReproduceNot required
TagsNo tags attached.
Test case numberNot needed

Relationships

related to 0022777 closedbugmaster Open CASCADE Visualization - Unsafe way to get attribute values from MeshVS_Drawer 

Activities

abv

2019-02-23 22:07

manager   ~0082321

Hello Benjamin,

Thank you for proposing this improvement, and sorry for late reply!

Indeed it would be good to have this kind of attribute, and we will appreciate if you provide such an improvement.

To support different compilers in compatible and convenient way, please define a macro similar to Standard_OVERRIDE or Standard_FALLTHROUGH (see file Standard_Macro.hxx). I believe the new macro can be naturally called Standard_NODISCARD.

git

2019-02-25 13:22

administrator   ~0082329

Branch CR30448 has been created by BenjaminBihler.

SHA-1: 2b2aab68f508d799d94f22299f968be7a7859533


Detailed log of new commits:

Author: Benjamin Bihler
Date: Mon Feb 25 11:16:24 2019 +0100

    0030448: Add Typo Detection to Derivation Creation Methods
    
    Using Standard_NODISCARD macro for derivation creation methods.

Author: Benjamin Bihler
Date: Mon Feb 25 10:52:01 2019 +0100

    0030448: Add Typo Detection to Derivation Creation Methods
    
    Added Standard_NODISCARD macro.

BenjaminBihler

2019-02-25 13:38

developer   ~0082330

Attention! After introducing the new attribute, there are several warnings issued during OCCT compilation. It seems to me as if they were appropriate warnings and the code should be corrected, but I haven't done this.

kgv

2019-02-25 14:07

developer   ~0082337 

-  Standard_EXPORT Handle(Geom_Geometry) Mirrored (const gp_Pnt& P) const;
+  Standard_EXPORT Standard_NODISCARD
+  Handle(Geom_Geometry) Mirrored (const gp_Pnt& P) const;

Although declaration grows cumbersome, I don't think it is a good idea splitting the line in such places.

BenjaminBihler

2019-02-25 14:19

developer   ~0082338

Kirill, I do prefer shorter lines (eases diffs). But since this is a matter of taste and fixing is simple, I can change it. Shall I?

kgv

2019-02-25 14:26

developer   ~0082339

Last edited: 2019-02-25 14:30

 Standard_NODISCARD is defined incorrectly:
-- The CXX compiler identification is GNU 4.9.2
...
/dn62/builds/CR30448-master-KGV/OCCT_SRC/src/Standard/Standard_Macro.hxx:67:56: error: missing binary operator before token "("
 #if defined(__has_cpp_attribute) && __has_cpp_attribute(nodiscard)



-- The CXX compiler identification is AppleClang 9.0.0.9000037
...
/Users/mnt/builds/CR30448-master-KGV/OCCT_SRC/src/Standard/Standard_Macro.hxx:67:37: error: function-like macro '__has_cpp_attribute' is not defined
#if defined(__has_cpp_attribute) && __has_cpp_attribute(nodiscard)


git

2019-02-25 15:08

administrator   ~0082340

Branch CR30448 has been updated by BenjaminBihler.

SHA-1: 6086a3f8c45bf5ecf8457353c9bc8f7e097dcf1f


Detailed log of new commits:

Author: Benjamin Bihler
Date: Mon Feb 25 13:03:00 2019 +0100

    0030448: Add Typo Detection to Derivation Creation Methods
    
    Fixed compilation problems with compilers that do not support the macro
    __has_cpp_attribute.

BenjaminBihler

2019-02-25 15:09

developer   ~0082341

I hope that your second remark is fixed now (I don't have a compiler to test it here), as regards the first remark I still wait for an answer. :-)

kgv

2019-02-25 15:59

developer   ~0082342

Last edited: 2019-02-25 16:00

 The corresponding attribute for GCC/CLang before C++17 should be "warn_unused_result".
#elif defined(__GNUC__)
  #define Standard_NODISCARD __attribute__((warn_unused_result))


Old MSVC had "_Check_return_" but it required /analyze compiler option.

abv

2019-02-25 16:15

manager   ~0082344

Hello Benjamin,

My own preference is to keep definitions like Standard_EXPORT and Standard_NODISCARD on the same line as the method itself, unless the line becomes really long. Note that in our Coding Rules we set 120 as recommended maximal length (https://dev.opencascade.org/doc/overview/html/occt_dev_guides__coding_rules.html#occt_coding_rules_3):

"Try to stay within the limit of 120 characters per line in all sources."

Besides, is there some reason why you did not add the new macro to operators, like operator-() in gp_Mat2d.hxx?

git

2019-02-25 17:27

administrator   ~0082345

Branch CR30448 has been updated by BenjaminBihler.

SHA-1: 6be70cacb6a4f6c3a4ae71ee8dcd439c6fd68d51


Detailed log of new commits:

Author: Benjamin Bihler
Date: Mon Feb 25 15:11:08 2019 +0100

    0030448: Add Typo Detection to Derivation Creation Methods
    
    Marked operators with Standard_NODISCARD, if they create derivations and are
    defined close to already marked derivation creation methods.

Author: Benjamin Bihler
Date: Mon Feb 25 14:43:07 2019 +0100

    0030448: Add Typo Detection to Derivation Creation Methods
    
    Switched to keeping attributes on method definition line where the line
    does not become excessively long.

BenjaminBihler

2019-02-25 17:28

developer   ~0082346

Andrey, there is probably a vast amount of methods to which this attribute could be added, but the typo danger ("Reverse" <-> "Reversed") is not present for the operators. Still I have adapted them if they have been close to the other method definition to be somehow consistent.

Kirill, I see the advantage of "warn_unused_result" for old g++ compilers/standards, but since I do not have old g++ versions for testing here, I won't add it. I guess that if you want to add it, you should add it twice: as an else branch if __has_cpp_attribute is not present and as an else branch if the "nodiscard" attribute is not present.

git

2019-02-26 10:55

administrator   ~0082352

Branch CR30448 has been updated forcibly by abv.

SHA-1: fcb6b7c4c0553d8b6f37eea3b8dad58f74924fc6

abv

2019-02-26 10:57

manager   ~0082353

I have added treatmnent of older GCC (hopefully will work also for CLang) and collapsed all changes in one commits.

git

2019-02-26 11:10

administrator   ~0082355

Branch CR30448 has been updated forcibly by abv.

SHA-1: f50a2aa3999bfa88021ec4e625117dc8fab926e4

git

2019-02-26 12:29

administrator   ~0082360

Branch CR30448 has been updated forcibly by abv.

SHA-1: 0504d70dadee3ed88579f813833cb5384b443c4e

git

2019-02-26 12:54

administrator   ~0082363

Branch CR30448 has been updated by abv.

SHA-1: c54e585f946e52ed51692316c784274b99ab6f5e


Detailed log of new commits:

Author: abv
Date: Tue Feb 26 12:39:32 2019 +0300

    0030448: Coding - add typo detection to derivation creation methods using Standard_NODISCARD attribute -- fixes
    
    Corrected code where warnings on unused result of calls to methods creating new objects are generated.
    In most cases it looks like spelling errors (e.g. Normalised() instead of Normalise())

git

2019-02-26 15:00

administrator   ~0082373

Branch CR30448 has been updated forcibly by abv.

SHA-1: 9bbcd742def6683b5048f7dc62f493316248ae35

abv

2019-02-26 19:24

manager   ~0082391

Mikhail, please review corrections in the last commit of the branch CR30448, made in modeling algorithms where the code was clearly wrong. These places are highlighted by new compiler warning (in our Jenkins tests issued by CLang on macOS only) activated by the change in the first commit.

The tests are OK, see Jenkins job CR30448-CR30448-abv.

Note that change in IntImpParGen.cxx (removal of useless statement) is justified by tests: first I tried to replace it by consequent call to Norm.Normalize(), but this caused failures in test bugs modalg_6 bug25908 and bug27341_206. (In the latter case, failure consisted in duplication of the longer edge on the right side.)

The changes in the current commit did not cause any deviations in tests.

msv

2019-02-27 18:20

developer   ~0082425

The strange code. But as far as tests approve, let's leave it as it is.
Reviewed.

apn

2019-02-27 18:57

administrator   ~0082426

Combination -
OCCT branch : CR30448
master SHA - 9bbcd742def6683b5048f7dc62f493316248ae35
d67d4b811012eef8913d3c535c29654d0acf3c4c
Products branch : CR30448 SHA - 09a08bc09d1b090f3c8f6582381475017d5e7093
was compiled on Linux, MacOS and Windows platforms and tested in optimize mode.

Number of compiler warnings:
No new/fixed warnings

Regressions/Differences/Improvements:
No regressions/differences

CPU differences:
Debian80-64:
OCCT
Total CPU difference: 16549.47000000002 / 16542.15999999996 [+0.04%]
Products
Total CPU difference: 9078.910000000034 / 9061.510000000035 [+0.19%]
Windows-64-VC14:
OCCT
Total CPU difference: 17954.015625 / 17946.09375 [+0.04%]
Products
Total CPU difference: 10497.71875 / 10439.25 [+0.56%]

Image differences :
No differences that require special attention

Memory differences :
No differences that require special attention

git

2019-03-05 13:46

administrator   ~0082628

Branch CR30448 has been deleted by kgv.

SHA-1: 9bbcd742def6683b5048f7dc62f493316248ae35

Related Changesets

occt: master 0be7dbe1

2019-02-25 09:52:01

BenjaminBihler


Committer: apn Details Diff		 0030448: Coding - add typo detection to derivation creation methods using Standard_NODISCARD attribute

Added macro Standard_NODISCARD equivalent to C++17 attribute [[nodiscard]] for compilers that support this.
Using Standard_NODISCARD macro for methods that create new object in gp, math, Geom, Bnd packages.
Marked equivalent operators with Standard_NODISCARD, if they are defined close to relevant methods.

Corrected code where warnings on unused result of calls to methods creating new objects are generated.
In most cases it looks like spelling errors (e.g. Normalised() instead of Normalise())		 Affected Issues
0030448
mod - dox/resources/occt_rm.doxyfile Diff File
mod - src/Bnd/Bnd_B2d.hxx Diff File
mod - src/Bnd/Bnd_B2f.hxx Diff File
mod - src/Bnd/Bnd_B3d.hxx Diff File
mod - src/Bnd/Bnd_B3f.hxx Diff File
mod - src/Bnd/Bnd_Box.hxx Diff File
mod - src/Bnd/Bnd_Box2d.hxx Diff File
mod - src/ChFi3d/ChFi3d_Builder_2.cxx Diff File
mod - src/Contap/Contap_ArcFunction.cxx Diff File
mod - src/Extrema/Extrema_ExtPElS.cxx Diff File
mod - src/Geom/Geom_Axis1Placement.hxx Diff File
mod - src/Geom/Geom_Curve.hxx Diff File
mod - src/Geom/Geom_Geometry.hxx Diff File
mod - src/Geom/Geom_Surface.hxx Diff File
mod - src/Geom/Geom_Transformation.hxx Diff File
mod - src/Geom/Geom_Vector.hxx Diff File
mod - src/Geom/Geom_VectorWithMagnitude.hxx Diff File
mod - src/Geom2d/Geom2d_AxisPlacement.hxx Diff File
mod - src/Geom2d/Geom2d_Curve.hxx Diff File
mod - src/Geom2d/Geom2d_Geometry.hxx Diff File
mod - src/Geom2d/Geom2d_Transformation.hxx Diff File
mod - src/Geom2d/Geom2d_Vector.hxx Diff File
mod - src/Geom2d/Geom2d_VectorWithMagnitude.cxx Diff File
mod - src/Geom2d/Geom2d_VectorWithMagnitude.hxx Diff File
mod - src/GeomFill/GeomFill_GuideTrihedronPlan.cxx Diff File
mod - src/GeomFill/GeomFill_TgtOnCoons.cxx Diff File
mod - src/gp/gp_Ax1.hxx Diff File
mod - src/gp/gp_Ax2.hxx Diff File
mod - src/gp/gp_Ax22d.hxx Diff File
mod - src/gp/gp_Ax2d.hxx Diff File
mod - src/gp/gp_Ax3.hxx Diff File
mod - src/gp/gp_Circ.hxx Diff File
mod - src/gp/gp_Circ2d.hxx Diff File
mod - src/gp/gp_Cone.hxx Diff File
mod - src/gp/gp_Cylinder.hxx Diff File
mod - src/gp/gp_Dir.hxx Diff File
mod - src/gp/gp_Dir2d.hxx Diff File
mod - src/gp/gp_Elips.hxx Diff File
mod - src/gp/gp_Elips2d.hxx Diff File
mod - src/gp/gp_GTrsf.hxx Diff File
mod - src/gp/gp_GTrsf2d.hxx Diff File
mod - src/gp/gp_Hypr.hxx Diff File
mod - src/gp/gp_Hypr2d.hxx Diff File
mod - src/gp/gp_Lin.hxx Diff File
mod - src/gp/gp_Lin2d.hxx Diff File
mod - src/gp/gp_Mat.hxx Diff File
mod - src/gp/gp_Mat2d.hxx Diff File
mod - src/gp/gp_Parab.hxx Diff File
mod - src/gp/gp_Parab2d.hxx Diff File
mod - src/gp/gp_Pln.hxx Diff File
mod - src/gp/gp_Pnt.hxx Diff File
mod - src/gp/gp_Pnt2d.hxx Diff File
mod - src/gp/gp_Quaternion.hxx Diff File
mod - src/gp/gp_Sphere.hxx Diff File
mod - src/gp/gp_Torus.hxx Diff File
mod - src/gp/gp_Trsf.hxx Diff File
mod - src/gp/gp_Trsf2d.hxx Diff File
mod - src/gp/gp_Vec.hxx Diff File
mod - src/gp/gp_Vec2d.hxx Diff File
mod - src/gp/gp_XY.hxx Diff File
mod - src/gp/gp_XYZ.hxx Diff File
mod - src/IntImpParGen/IntImpParGen.cxx Diff File
mod - src/IntImpParGen/IntImpParGen_Tool.cxx Diff File
mod - src/math/math_IntegerVector.hxx Diff File
mod - src/math/math_Matrix.hxx Diff File
mod - src/math/math_Vector.hxx Diff File
mod - src/ShapeAnalysis/ShapeAnalysis_TransferParametersProj.cxx Diff File
mod - src/Standard/Standard_Macro.hxx Diff File
mod - src/TopLoc/TopLoc_Location.hxx Diff File
mod - src/Units/Units_Token.hxx Diff File

Issue History

Date Modified Username Field Change
2019-01-18 16:43 BenjaminBihler New Issue
2019-01-18 16:43 BenjaminBihler Assigned To => abv
2019-01-18 16:44 BenjaminBihler Status new => feedback
2019-02-23 22:07 abv Note Added: 0082321
2019-02-23 22:08 abv Assigned To abv => BenjaminBihler
2019-02-25 13:22 git Note Added: 0082329
2019-02-25 13:38 BenjaminBihler Note Added: 0082330
2019-02-25 13:38 BenjaminBihler Assigned To BenjaminBihler => abv
2019-02-25 13:38 BenjaminBihler Status feedback => resolved
2019-02-25 13:38 BenjaminBihler Steps to Reproduce Updated
2019-02-25 14:07 kgv Note Added: 0082337
2019-02-25 14:10 kgv Category OCCT:Foundation Classes => OCCT:Coding
2019-02-25 14:10 kgv Summary Add Typo Detection to Derivation Creation Methods => Coding - add typo detection to derivation creation methods using Standard_NODISCARD attribute
2019-02-25 14:19 BenjaminBihler Note Added: 0082338
2019-02-25 14:26 kgv Note Added: 0082339
2019-02-25 14:30 kgv Note Edited: 0082339
2019-02-25 15:08 git Note Added: 0082340
2019-02-25 15:09 BenjaminBihler Note Added: 0082341
2019-02-25 15:59 kgv Note Added: 0082342
2019-02-25 16:00 kgv Note Edited: 0082342
2019-02-25 16:00 kgv Note Edited: 0082342
2019-02-25 16:15 abv Note Added: 0082344
2019-02-25 17:27 git Note Added: 0082345
2019-02-25 17:28 BenjaminBihler Note Added: 0082346
2019-02-26 10:55 git Note Added: 0082352
2019-02-26 10:57 abv Note Added: 0082353
2019-02-26 11:10 git Note Added: 0082355
2019-02-26 12:29 git Note Added: 0082360
2019-02-26 12:54 git Note Added: 0082363
2019-02-26 15:00 git Note Added: 0082373
2019-02-26 18:24 abv Assigned To abv => msv
2019-02-26 19:24 abv Note Added: 0082391
2019-02-27 18:20 msv Note Added: 0082425
2019-02-27 18:20 msv Assigned To msv => bugmaster
2019-02-27 18:20 msv Status resolved => reviewed
2019-02-27 18:57 apn Test case number => Not needed
2019-02-27 18:57 apn Note Added: 0082426
2019-02-27 18:57 apn Status reviewed => tested
2019-03-03 21:12 apn Changeset attached => occt master 0be7dbe1
2019-03-03 21:12 apn Assigned To bugmaster => apn
2019-03-03 21:12 apn Status tested => verified
2019-03-03 21:12 apn Resolution open => fixed
2019-03-05 13:46 git Note Added: 0082628
2019-08-27 14:39 kgv Relationship added related to 0022777