Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bravo team cppcheck set 41 #38731

Merged
merged 12 commits into from
Feb 7, 2025
Merged

Bravo team cppcheck set 41 #38731

merged 12 commits into from
Feb 7, 2025

Conversation

sf1919
Copy link
Contributor

@sf1919 sf1919 commented Jan 28, 2025

Description of work

Please note that the FitParameter.cpp fix is in a separate PR (#38745 )

Summary of work

Purpose of work

Issue File Line
knownConditionTrueFalse ${CMAKE_SOURCE_DIR}/Framework/Geometry/src/Instrument/FitParameter.cpp 54
constParameterReference ${CMAKE_SOURCE_DIR}/Framework/Geometry/src/Instrument/InstrumentDefinitionParser.cpp 593
unreadVariable ${CMAKE_SOURCE_DIR}/Framework/Geometry/src/Instrument/InstrumentDefinitionParser.cpp 1552
constVariablePointer ${CMAKE_SOURCE_DIR}/Framework/Geometry/src/Instrument/InstrumentDefinitionParser.cpp 2330
constParameterReference ${CMAKE_SOURCE_DIR}/Framework/Geometry/src/Instrument/InstrumentDefinitionParser.cpp 2640
constVariablePointer ${CMAKE_SOURCE_DIR}/Framework/Geometry/src/Instrument/InstrumentDefinitionParser.cpp 2677
constParameterPointer ${CMAKE_SOURCE_DIR}/Framework/Geometry/src/Instrument/InstrumentDefinitionParser.cpp 3059
constVariablePointer ${CMAKE_SOURCE_DIR}/Framework/Geometry/src/Instrument/ObjCompAssembly.cpp 346
constParameterPointer ${CMAKE_SOURCE_DIR}/Framework/Geometry/src/Instrument/XMLInstrumentParameter.cpp 92
stlIfStrFind ${CMAKE_SOURCE_DIR}/Framework/Geometry/src/Instrument/XMLInstrumentParameter.cpp 146
knownConditionTrueFalse ${CMAKE_SOURCE_DIR}/Framework/Geometry/src/Math/Acomp.cpp 165
invalidContainer ${CMAKE_SOURCE_DIR}/Framework/Geometry/src/Math/Acomp.cpp 495
knownConditionTrueFalse ${CMAKE_SOURCE_DIR}/Framework/Geometry/src/Objects/CSGObject.cpp 143
knownConditionTrueFalse ${CMAKE_SOURCE_DIR}/Framework/Geometry/src/Objects/CSGObject.cpp 320

There is no associated issue.

Further detail of work

To test:


Reviewer

Please comment on the points listed below (full description).
Your comments will be used as part of the gatekeeper process, so please comment clearly on what you have checked during your review. If changes are made to the PR during the review process then your final comment will be the most important for gatekeepers. In this comment you should make it clear why any earlier review is still valid, or confirm that all requested changes have been addressed.

Code Review

  • Is the code of an acceptable quality?
  • Does the code conform to the coding standards?
  • Are the unit tests small and test the class in isolation?
  • If there is GUI work does it follow the GUI standards?
  • If there are changes in the release notes then do they describe the changes appropriately?
  • Do the release notes conform to the release notes guide?

Functional Tests

  • Do changes function as described? Add comments below that describe the tests performed?
  • Do the changes handle unexpected situations, e.g. bad input?
  • Has the relevant (user and developer) documentation been added/updated?

Does everything look good? Mark the review as Approve. A member of @mantidproject/gatekeepers will take care of it.

Gatekeeper

If you need to request changes to a PR then please add a comment and set the review status to "Request changes". This will stop the PR from showing up in the list for other gatekeepers.

@github-actions github-actions bot added the Has Conflicts Used by the bot to label pull requests that have conflicts label Jan 28, 2025
Copy link

👋 Hi, @sf1919,

Conflicts have been detected against the base branch. Please rebase your branch against the base branch.

@github-actions github-actions bot removed the Has Conflicts Used by the bot to label pull requests that have conflicts label Jan 29, 2025
@sf1919 sf1919 added the Maintenance Unassigned issues to be addressed in the next maintenance period. label Jan 29, 2025
@sf1919 sf1919 added this to the Release 6.13 milestone Jan 29, 2025
@sf1919 sf1919 marked this pull request as ready for review January 29, 2025 14:18
@yusufjimoh yusufjimoh self-assigned this Jan 29, 2025
@sf1919 sf1919 force-pushed the bravo_team_cppcheck_set_41 branch from eac1345 to fd1d33e Compare January 29, 2025 14:54
Copy link
Contributor

@yusufjimoh yusufjimoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this. All the changes look good. I just noticed that the issue on line 473 in {CMAKE_SOURCE_DIR}/Framework/Geometry/src/Objects/CSGObject.cpp wasn’t covered in this pull request, though it was mentioned in the PR description. I was wondering if we should update the description to reflect this?

@sf1919
Copy link
Contributor Author

sf1919 commented Jan 30, 2025

Thanks @yusufjimoh I will update the description. That failure was fixed in a previous set (see #38715) which is part of the reason I picked up this set to avoid confusion.

yusufjimoh
yusufjimoh previously approved these changes Jan 30, 2025
@jclarkeSTFC jclarkeSTFC self-assigned this Jan 30, 2025
Copy link
Contributor

@jclarkeSTFC jclarkeSTFC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I think that within a file we should stick to the existing convention in that file for either east or west const, so we don't have a mix. I put a suggestion for one or two, but would you mind checking them all please? Also I think we can remove one of the inline suppressions.

@@ -189,7 +189,7 @@ class MANTID_GEOMETRY_DLL InstrumentDefinitionParser {
/// elements with \<cuboid\>'s
/// (note for now this will only work for \<cuboid\>'s and when necessary this
/// can be extended).
void adjust(Poco::XML::Element *pElem, std::map<std::string, bool> &isTypeAssembly,
void adjust(Poco::XML::Element *pElem, std::map<std::string, bool> const &isTypeAssembly,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind keeping to west-const for consistency within the file please? On line 71 you've got const std::string but here you've got std::map const. It doesn't make any difference to the compiler but I think it's good to keep it consistent with a file. Line 254 also east instead of west.

@@ -76,7 +76,7 @@ class MANTID_GEOMETRY_DLL XMLInstrumentParameter {

/// Returns parameter value as generated using possibly equation expression
/// etc
double createParamValue(Mantid::Kernel::TimeSeriesProperty<double> *logData, const Kernel::TimeROI *) const;
double createParamValue(Mantid::Kernel::TimeSeriesProperty<double> const *logData, const Kernel::TimeROI *) const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
double createParamValue(Mantid::Kernel::TimeSeriesProperty<double> const *logData, const Kernel::TimeROI *) const;
double createParamValue(const Mantid::Kernel::TimeSeriesProperty<double> *logData, const Kernel::TimeROI *) const;

Comment on lines 147 to 149
// cppcheck-suppress stlIfStrFind as string::starts_with(), cppcheck's suggested change, does not allow for position
// 0 to be anywhere
else if (m_extractSingleValueAs.find("position") == 0 && m_extractSingleValueAs.size() >= 10) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the original code is checking that m_extractSingleValueAs has the string "position" starting at index 0, so starts_with should be okay.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was failing a number of tests when I used starts_with instead so I don't think it is necessarily the way it works

Copy link
Contributor

@jclarkeSTFC jclarkeSTFC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the changes, spotted a couple more things, sorry.

@github-actions github-actions bot added the Has Conflicts Used by the bot to label pull requests that have conflicts label Jan 31, 2025
Copy link

👋 Hi, @sf1919,

Conflicts have been detected against the base branch. Please rebase your branch against the base branch.

- fix knownConditionTrueFalse and invalidContainer suppressions
- Fix erros in fixing Acomp and CSGObjects from previous commits
- Fix constParameterReference and constVariablePointer instances in InstrumentDefinitionParser
- Remove the shape code from the Structured Detector in InstrumentDefinitionParser. The shape is defined within StructuredDetector.cpp instead (probably a copy/paste error)
- fixed constParameterPointer and stlIfStrFind
- cppcheck suggests using string::starts_with(). However this breaks functionality because position 0 can be anywhere. Although inefficient the find is necessary as it is.
@sf1919 sf1919 force-pushed the bravo_team_cppcheck_set_41 branch from 881dc35 to 326ff87 Compare February 7, 2025 11:06
@github-actions github-actions bot removed the Has Conflicts Used by the bot to label pull requests that have conflicts label Feb 7, 2025
sf1919 and others added 3 commits February 7, 2025 11:08
Co-authored-by: James Clarke <139879523+jclarkeSTFC@users.noreply.github.com>
Co-authored-by: James Clarke <139879523+jclarkeSTFC@users.noreply.github.com>
Co-authored-by: James Clarke <139879523+jclarkeSTFC@users.noreply.github.com>
Copy link
Contributor

@jclarkeSTFC jclarkeSTFC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making those changes 👍

@jclarkeSTFC jclarkeSTFC merged commit 0c87c51 into main Feb 7, 2025
10 checks passed
@jclarkeSTFC jclarkeSTFC deleted the bravo_team_cppcheck_set_41 branch February 7, 2025 15:40
rboston628 pushed a commit that referenced this pull request Feb 10, 2025
* Fixing knownConditionTrueFalse suppressions in CSGObject

* Fix constVariablePointer suppression in ObjCompAssembly

* Fix cppcheck suppressions in Acomp

- fix knownConditionTrueFalse and invalidContainer suppressions

* Fix cppcheck suppressions

- Fix erros in fixing Acomp and CSGObjects from previous commits
- Fix constParameterReference and constVariablePointer instances in InstrumentDefinitionParser
- Remove the shape code from the Structured Detector in InstrumentDefinitionParser. The shape is defined within StructuredDetector.cpp instead (probably a copy/paste error)

* Fix cppcheck suppressions in XMLInstrumentParameter

- fixed constParameterPointer and stlIfStrFind

* Fix oustanding constVariablePointer

* Use inline suppression for stlIfStrFind

- cppcheck suggests using string::starts_with(). However this breaks functionality because position 0 can be anywhere. Although inefficient the find is necessary as it is.

* Use west const more consistently

* Change the call to find to starts_with to improve searching

* Update Framework/Geometry/src/Instrument/XMLInstrumentParameter.cpp

Co-authored-by: James Clarke <139879523+jclarkeSTFC@users.noreply.github.com>

* Update Framework/Geometry/src/Instrument/InstrumentDefinitionParser.cpp

Co-authored-by: James Clarke <139879523+jclarkeSTFC@users.noreply.github.com>

* Update Framework/Geometry/src/Instrument/InstrumentDefinitionParser.cpp

Co-authored-by: James Clarke <139879523+jclarkeSTFC@users.noreply.github.com>

---------

Co-authored-by: James Clarke <139879523+jclarkeSTFC@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Unassigned issues to be addressed in the next maintenance period.
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants