Cppcheck 2.15 #5282

jmarrec · 2024-10-29T16:38:37Z

Pull request overview

Prompted by recent failures in our cppcheck workflow, use the latest version, and fix more issues that are being caught.

Pull Request Author

Model API Changes / Additions
Any new or modified fields have been implemented in the EnergyPlus ForwardTranslator (and ReverseTranslator as appropriate)
Model API methods are tested (in src/model/test)
EnergyPlus ForwardTranslator Tests (in src/energyplus/Test)
If a new object or method, added a test in NREL/OpenStudio-resources: Add Link
If needed, added VersionTranslation rules for the objects (src/osversion/VersionTranslator.cpp)
Verified that C# bindings built fine on Windows, partial classes used as needed, etc.
All new and existing tests passes
If methods have been deprecated, update rest of code to use the new methods

Labels:

If change to an IDD file, add the label IDDChange
If breaking existing API, add the label APIChange
If deemed ready, add label Pull Request - Ready for CI so that CI builds your PR

Review Checklist

This will not be exhaustively relevant to every PR.

Perform a Code Review on GitHub
Code Style, strip trailing whitespace, etc.
All related changes have been implemented: model changes, model tests, FT changes, FT tests, VersionTranslation, OS App
Labeling is ok
If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
If feature, test running new feature, try creative ways to break it
CI status: all green or justified

…on/ruby engines etc

…ocumentation purposes

…s a vector, the Surface/SubSurface return an optional The PlanarSurface should be changed or removed.

jmarrec · 2024-10-29T16:39:24Z

.github/workflows/cppcheck.yml

+            -i src/cli/test/ \
+            -i src/airflow/contam/ \
+            -i src/polypartition/ \
+            -i src/nano/ \
+            . \


This effectively adds the root directories "python" and "ruby" to cppcheck

jmarrec · 2024-10-29T16:39:59Z

src/alfalfa/AlfalfaJSON.hpp

@@ -100,7 +100,7 @@ namespace alfalfa {
    /**
     * Get a vector of all points currently exported to the Alfalfa API
     */
-    std::vector<AlfalfaPoint> points();
+    std::vector<AlfalfaPoint> points() const;


cppcheck 2.13 already complains about this one on develop right now

jmarrec · 2024-10-29T16:41:00Z

src/model/Building.hpp

@@ -48,7 +48,7 @@ namespace model {
    /** @name Constructors and Destructors */
    //@{

-    virtual ~Building() = default;
+    virtual ~Building() override = default;


lots of this.

jmarrec · 2024-10-29T16:41:49Z

src/model/SetpointManagerColdest.hpp

@@ -48,22 +48,16 @@ namespace model {
    /** @name Getters */
    //@{

-    std::string controlVariable() const;


Lots of duplInheritedMembers fixes

jmarrec · 2024-10-29T16:42:38Z

src/model/StraightComponent.hpp

+     *
+     *  Reimplemented from HVACComponent.
+     */
+    boost::optional<AirLoopHVAC> airLoopHVAC() const; // cppcheck-suppress [duplInheritedMember] for documentation purposes


Some I keep, for documentation purposes mostly, because the docstring is helpful versus the base class one.

jmarrec · 2024-10-29T16:48:42Z

src/utilities/geometry/Polyhedron.cpp

@@ -95,7 +95,7 @@ void Surface3dEdge::resetEdgeMatching() {
  m_conflictedOrientation = false;
 }

-bool Surface3dEdge::containsPoint(const Point3d& testVertex) {
+bool Surface3dEdge::containsPoint(const Point3d& testVertex) const {


const correctness

jmarrec · 2024-10-29T16:48:46Z

src/utilities/geometry/Polyhedron.cpp

@@ -396,7 +396,7 @@ std::vector<Surface3dEdge> Polyhedron::edgesNotTwo(bool includeCreatedEdges) con
      }
    }
  }
-  for (auto& edge : edgesNotTwo) {
+  for (const auto& edge : edgesNotTwo) {


const correctness

jmarrec · 2024-10-29T16:49:07Z

src/utilities/idd/IddFieldProperties.cpp

@@ -184,7 +184,7 @@ std::ostream& IddFieldProperties::print(std::ostream& os) const {
      os << "       \\reference " << reference << '\n';
    }
  }
-  if (!references.empty()) {
+  if (!referenceClassNames.empty()) {


I think this was a mistake to begin with (copy paste)

jmarrec · 2024-10-29T16:49:20Z

src/utilities/idd/IddObject.cpp

    string propertiesText;
    if (boost::regex_search(text, matches, iddRegex::line())) {
-      objectName = string(matches[1].first, matches[1].second);
+      std::string objectName = string(matches[1].first, matches[1].second);


reducing scope

jmarrec · 2024-10-29T16:51:22Z

src/model/SubSurface.hpp

+    // cppcheck-suppress [duplInheritedMember] because PlanarSurface is dumb and returns a vector
    boost::optional<SurfacePropertyConvectionCoefficients> surfacePropertyConvectionCoefficients() const;


I didn't want to break API, but this is clearly wrong.

This is wrong:

OpenStudio/src/model/PlanarSurface.hpp

Lines 199 to 200 in 62f977e

/// Returns any SurfacePropertyConvectionCoefficients associated with this surface, does not return SurfacePropertyConvectionCoefficientsMultipleSurface.

std::vector<SurfacePropertyConvectionCoefficients> surfacePropertyConvectionCoefficients() const;

ci-commercialbuildings · 2024-10-29T17:32:23Z

CI Results for 21d7036:

Ubuntu 24.04:
Ubuntu 22.04:
Darwin:
Windows:
- OpenStudio-3.9.0-alpha+21d7036650-Windows.exe
- OpenStudio-3.9.0-alpha+21d7036650-Windows.tar.gz
Code Coverage:

wenyikuang · 2024-10-31T04:28:14Z

ruby/interpreter/RubyEval.hpp

-#include "ruby.h"
-#include "./RubyException.hpp"
+#include <ruby.h>
+#include "RubyException.hpp"


Just curios , why are using relative path above but keeps the hpp name here?

I'm not sure I understand the question, could you rephrase please?

jmarrec added 10 commits October 29, 2024 17:37

Start pleasing cppcheck 2.15

f49e76d

tweak cppcheck config and ignore the iddObjectType duplInheritedMember

55e9e9a

change cppcheck to scan entire root dir and not src, so we catch pyth…

b81f76c

…on/ruby engines etc

keep fixing stuff

f0cff49

Fixup from duplInheritedMembers

3407f7e

Resolve more duplInheritedMembers... Some I remove, some I keep for d…

bc2b590

…ocumentation purposes

Last one, PlanarSurface::surfacePropertyConvectionCoefficients return…

fb16797

…s a vector, the Surface/SubSurface return an optional The PlanarSurface should be changed or removed.

Const correctness for Alfalfa after rebase

fe41009

Use ubuntu noble, my PPA has a cppcheck 2.15 prebuilt

799d4d1

Fixup execAndThrowAndError

834fa9c

jmarrec added Developer Issue Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. labels Oct 29, 2024

jmarrec self-assigned this Oct 29, 2024

jmarrec commented Oct 29, 2024

View reviewed changes

clang format 14 (I have 18 on my machine)

21d7036

jmarrec requested review from kbenne and wenyikuang October 30, 2024 10:48

wenyikuang reviewed Oct 31, 2024

View reviewed changes

wenyikuang merged commit ed4415f into develop Oct 31, 2024
5 of 6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cppcheck 2.15 #5282

Cppcheck 2.15 #5282

jmarrec commented Oct 29, 2024

jmarrec Oct 29, 2024

jmarrec Oct 29, 2024

jmarrec Oct 29, 2024

jmarrec Oct 29, 2024

jmarrec Oct 29, 2024

jmarrec Oct 29, 2024

jmarrec Oct 29, 2024

jmarrec Oct 29, 2024

jmarrec Oct 29, 2024

jmarrec Oct 29, 2024

ci-commercialbuildings commented Oct 29, 2024 •

edited

Loading

wenyikuang Oct 31, 2024

jmarrec Oct 31, 2024

		// cppcheck-suppress [duplInheritedMember] because PlanarSurface is dumb and returns a vector
		boost::optional<SurfacePropertyConvectionCoefficients> surfacePropertyConvectionCoefficients() const;

	/// Returns any SurfacePropertyConvectionCoefficients associated with this surface, does not return SurfacePropertyConvectionCoefficientsMultipleSurface.
	std::vector<SurfacePropertyConvectionCoefficients> surfacePropertyConvectionCoefficients() const;

Cppcheck 2.15 #5282

Cppcheck 2.15 #5282

Conversation

jmarrec commented Oct 29, 2024

Pull request overview

Pull Request Author

Review Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ci-commercialbuildings commented Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ci-commercialbuildings commented Oct 29, 2024 •

edited

Loading