WIP: Addressed arithmetic overflow and other code analysis warnings #331
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
mmarineBlueQuartz
commented
Mar 25, 2019
mmarineBlueQuartz
commented
- Initialized PipelineMessage index in constructors.
- Marked a possible coding error in ImportAsciiDataArray where two identical error conditions are checked using || operator.
- Fixed an incorrect cast in ReadASCIIData where a double is cast to a float for a method parameter that is expecting a double. Due to multiple calculations of the same value, it was saved to a constant before the first calculation was required.
- Changed a for loop variable in geometry compute methods to avoid default castings when dereferencing a pointer multiple times per iteration.
- Changed geometry requirements in addAttributeMatrix methods from using || for enum checks to &&. The previous versions could never be false and prevented the entire method call from doing anything.
- Fixed a possible overflow when calculating ImageGeom and RectGridGeom coords. The previous versions would have overflowed before casting to a larger size.
- Updated variables used to store min and max float values to constexpr to take advantage of the fact that they are known at compile time and are never modified.
- Removed unnecessary casts in SIMPLibRandom where doubles were cast to float and then saved as a double.
- Fixed ColorTable::GetColorTable due to the current implementation going out of array bounds. The 2D array float color[8][3] was being accessed with indices [8][2].
* Initialized PipelineMessage index * Marked a possible coding error in ImportAsciiDataArray where two identical error conditions are checked using || operator. * Fixed an incorrect cast in ReadASCIIData where a double is cast to a float for a method parameter that is expecting a double. Due to multiple calculations of the same value, it was saved to a constant before the first calculation was required. * Changed a for loop variable in geometry compute methods to avoid default castings when dereferencing a pointer multiple times per iteration. * Changed geometry requirements in addAttributeMatrix from using || for enum checks to &&. The previous versions could never be false and invalidated the entire method. * Fixed a possible overflow when calculating ImageGeom and RectGridGeom coords. The previous versions would have overflowed before casting to a larger size. * Updated variables used to store min and max float values to constexpr to take advantage of the fact that they are known at compile time and are never modified. * Removed unnecessary casts in SIMPLibRandom where doubles were cast to float and then saved as a double. * Fixed ColorTable::GetColorTable due to the current implementation going out of array bounds. The 2D array float color[8][3] was being accessed with indices [8][2].
* Changed xDim, yDim, and zDim parameters to size_t from int. If either one or three of them were negative, the total size variable used to allocate memory would save a negative number as an unsigned value, causing overflow. Moreover, any negative dimensions would silently prevent data from being read.
| return err; | ||
| } | ||
| if(err == RBR_READ_ERROR || err == RBR_READ_ERROR) | ||
| if(err == RBR_READ_ERROR || err == RBR_READ_ERROR) // Identical |
Contributor
There was a problem hiding this comment.
They are the same but I wonder if the code should be checking another error condition?
| return err; | ||
| } | ||
| if(err == RBR_READ_ERROR || err == RBR_READ_ERROR) | ||
| if(err == RBR_READ_ERROR || err == RBR_READ_ERROR) // Identical |
| float step = 1.0 / float(numColors); | ||
| float nodeStep = 1.0f / float(numColorNodes - 1); | ||
| for(int i = 0; i < (numColors); i++) | ||
| for(int i = 0; i < numColors; i++) |
Contributor
There was a problem hiding this comment.
Can you use new style C++ casts rather than old "C" style casts?
Contributor
|
Generally looks good. Wish I had a few of these before the release on Friday. We will get them on next months release. |
* Updated preset color table only.
* Other errors are already handled, so these seem to be redundant rather than typos.
* Applied fixes from preset color table method. * Updated incorrect currBinIndex maximum value in preset table method * Moved variables inside the loop where appropriate. * Updated numColors parameter to size_t both for cast issues and because it makes no sense to have a negative number of colors. * Made variables const where appropriate.
* Performed casts on getNumberOfComponents() to size_t for calculations. This method should probably return size_t instead of a signed int if a negative component dimension is not possible. * Changed DynamicTableData::ExpandData parameters nRows and nCols from signed int to size_t. Negative values were not handled, and the values are more descriptive with their current type. size_t should be prefered over int when it comes to dimensions unless there is a good reason to include negative values as a valid option. * Added a hsize_t cast to H5DataArrayWriter when calculating rank size in H5DataArrayWriter.hpp By casting the first value, arithmetic operations progress it forward while preventing int32_t-related overflow. * Added casts to int64_t when faceId values to local variables in GeometryMath.cpp. These variables were used for referencing vertex pointer indices where int64_t values were expected
* Added missing `f` in plane4Comp and plane7Comp arithmetic operations. This was determined as a mistake by looking at other operations where the same calculations were consistently done with a floating point constant, but the 'f' was left off in two places. The static_cast<float> was determined to silence a warning regarding type casting down from double to float due to the propogation of the double type through the calculation.
* Silenced unitialized variable warning
* This is unlikely to affect the default values for integers, floating point numbers, and booleans (not created with SIMPL_BOOL_PROPERTY). * Hard sets boolean properties to false using SIMPL_BOOL_PROPERTY * Silences uninitialized variable warnings for other types. * These macros still have the potential to cause bugs due to uninitialized variables.
* Initializes to 0 for the specified constructor.
Contributor
|
Can you rebase/merge the latest develop in with this branch and update the PR? |
# Conflicts: # Source/SIMPLib/CoreFilters/MassCreateData.cpp # Source/SIMPLib/CoreFilters/ReadASCIIData.cpp # Source/SIMPLib/Geometry/IGeometry.cpp # Source/SIMPLib/Geometry/ImageGeom.cpp
This reverts commit 77a6fb1.
# Conflicts: # Source/SIMPLib/CoreFilters/MassCreateData.cpp # Source/SIMPLib/CoreFilters/ReadASCIIData.cpp # Source/SIMPLib/Geometry/IGeometry.cpp # Source/SIMPLib/Geometry/ImageGeom.cpp
imikejackson
pushed a commit
that referenced
this pull request
Jul 16, 2019
* Addressed arithmetic overflow and other code analysis warnings * Initialized PipelineMessage index * Marked a possible coding error in ImportAsciiDataArray where two identical error conditions are checked using || operator. * Fixed an incorrect cast in ReadASCIIData where a double is cast to a float for a method parameter that is expecting a double. Due to multiple calculations of the same value, it was saved to a constant before the first calculation was required. * Changed a for loop variable in geometry compute methods to avoid default castings when dereferencing a pointer multiple times per iteration. * Changed geometry requirements in addAttributeMatrix from using || for enum checks to &&. The previous versions could never be false and invalidated the entire method. * Fixed a possible overflow when calculating ImageGeom and RectGridGeom coords. The previous versions would have overflowed before casting to a larger size. * Updated variables used to store min and max float values to constexpr to take advantage of the fact that they are known at compile time and are never modified. * Removed unnecessary casts in SIMPLibRandom where doubles were cast to float and then saved as a double. * Fixed ColorTable::GetColorTable due to the current implementation going out of array bounds. The 2D array float color[8][3] was being accessed with indices [8][2]. * Updated VTKFileReader::skipVolume to use size_t dimensions * Changed xDim, yDim, and zDim parameters to size_t from int. If either one or three of them were negative, the total size variable used to allocate memory would save a negative number as an unsigned value, causing overflow. Moreover, any negative dimensions would silently prevent data from being read. * Updated ColorTable::GetColorTable with static_cast and rename * Updated preset color table only. * Removed duplicate error checks in ImportAsciDataArray * Other errors are already handled, so these seem to be redundant rather than typos. * Updated ColorTable::GetColorTable for json values * Applied fixes from preset color table method. * Updated incorrect currBinIndex maximum value in preset table method * Moved variables inside the loop where appropriate. * Updated numColors parameter to size_t both for cast issues and because it makes no sense to have a negative number of colors. * Made variables const where appropriate. * Ran clang-format on ColorTable.cpp for readability * Fixed more arithmetic overflow warnings with type casts * Performed casts on getNumberOfComponents() to size_t for calculations. This method should probably return size_t instead of a signed int if a negative component dimension is not possible. * Changed DynamicTableData::ExpandData parameters nRows and nCols from signed int to size_t. Negative values were not handled, and the values are more descriptive with their current type. size_t should be preferred over int when it comes to dimensions unless there is a good reason to include negative values as a valid option. * Added a hsize_t cast to H5DataArrayWriter when calculating rank size in H5DataArrayWriter.hpp By casting the first value, arithmetic operations progress it forward while preventing int32_t-related overflow. * Added casts to int64_t when faceId values to local variables in GeometryMath.cpp. These variables were used for referencing vertex pointer indices where int64_t values were expected * Fixed a few uninitialized variables * Fixed type casting in CubeOctohedronOps.cpp * Added missing `f` in plane4Comp and plane7Comp arithmetic operations. This was determined as a mistake by looking at other operations where the same calculations were consistently done with a floating point constant, but the 'f' was left off in two places. The static_cast<float> was determined to silence a warning regarding type casting down from double to float due to the propagation of the double type through the calculation. * Added default value to ErrorObject::ok boolean in ParseFunctors * Silenced initialized variable warning * Added default construction to SIMPLib property macros * This is unlikely to affect the default values for integers, floating point numbers, and booleans (not created with SIMPL_BOOL_PROPERTY). * Hard sets boolean properties to false using SIMPL_BOOL_PROPERTY * Silences uninitialized variable warnings for other types. * These macros still have the potential to cause bugs due to uninitialized variables. * Added a default initialization for PipelineMessage code. * Initializes to 0 for the specified constructor. * Merge branch 'develop' into feature/FixCodeAnalysis # Conflicts: # Source/SIMPLib/CoreFilters/MassCreateData.cpp # Source/SIMPLib/CoreFilters/ReadASCIIData.cpp # Source/SIMPLib/Geometry/IGeometry.cpp # Source/SIMPLib/Geometry/ImageGeom.cpp Signed-off-by: Michael Jackson <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.