MantisBT
Mantis Bug Tracker Workflow

View Issue Details Jump to Notes ] Related Changesets ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0030448Community[OCCT] OCCT:Codingpublic2019-01-18 16:432019-08-27 14:39
ReporterBenjaminBihler 
Assigned Toapn 
PrioritynormalSeverityminor 
StatusverifiedResolutionfixed 
PlatformAOSLOS VersionL
Product Version[OCCT] 7.3.0 
Target Version[OCCT] 7.4.0Fixed in Version 
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
Attached Files

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

-  Notes
(0082321)
abv (manager)
2019-02-23 22:07

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.
(0082329)
git (administrator)
2019-02-25 13:22

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.
(0082330)
BenjaminBihler (developer)
2019-02-25 13:38

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.
(0082337)
kgv (developer)
2019-02-25 14:07

-  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.
(0082338)
BenjaminBihler (developer)
2019-02-25 14:19

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?
(0082339)
kgv (developer)
2019-02-25 14:26
edited on: 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)


(0082340)
git (administrator)
2019-02-25 15:08

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.

(0082341)
BenjaminBihler (developer)
2019-02-25 15:09

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. :-)
(0082342)
kgv (developer)
2019-02-25 15:59
edited on: 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.

(0082344)
abv (manager)
2019-02-25 16:15

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?
(0082345)
git (administrator)
2019-02-25 17:27

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.

(0082346)
BenjaminBihler (developer)
2019-02-25 17:28

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.
(0082352)
git (administrator)
2019-02-26 10:55

Branch CR30448 has been updated forcibly by abv.

SHA-1: fcb6b7c4c0553d8b6f37eea3b8dad58f74924fc6
(0082353)
abv (manager)
2019-02-26 10:57

I have added treatmnent of older GCC (hopefully will work also for CLang) and collapsed all changes in one commits.
(0082355)
git (administrator)
2019-02-26 11:10

Branch CR30448 has been updated forcibly by abv.

SHA-1: f50a2aa3999bfa88021ec4e625117dc8fab926e4
(0082360)
git (administrator)
2019-02-26 12:29

Branch CR30448 has been updated forcibly by abv.

SHA-1: 0504d70dadee3ed88579f813833cb5384b443c4e
(0082363)
git (administrator)
2019-02-26 12:54

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())

(0082373)
git (administrator)
2019-02-26 15:00

Branch CR30448 has been updated forcibly by abv.

SHA-1: 9bbcd742def6683b5048f7dc62f493316248ae35
(0082391)
abv (manager)
2019-02-26 19:24

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.
(0082425)
msv (developer)
2019-02-27 18:20

The strange code. But as far as tests approve, let's leave it as it is.
Reviewed.
(0082426)
apn (administrator)
2019-02-27 18:57

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
(0082628)
git (administrator)
2019-03-05 13:46

Branch CR30448 has been deleted by kgv.

SHA-1: 9bbcd742def6683b5048f7dc62f493316248ae35

- Related Changesets
occt: master 0be7dbe1
Timestamp: 2019-02-25 09:52:01
Author: 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())
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 View Revisions
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 View Revisions
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 View Revisions
2019-02-25 16:00 kgv Note Edited: 0082342 View Revisions
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


Copyright © 2000 - 2019 MantisBT Team
Powered by Mantis Bugtracker