Bravo team cppcheck set 41 (#38731)

* 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>

Author: sf1919

Framework/Geometry/inc/MantidGeometry/Instrument/InstrumentDefinitionParser.h

@@ -68,7 +68,7 @@ class MANTID_GEOMETRY_DLL InstrumentDefinitionParser {
   static std::string getNameOfLocationElement(const Poco::XML::Element *pElem, const Poco::XML::Element *pCompElem);
 
   /// Save DOM tree to xml file
-  void saveDOM_Tree(std::string &outFilename);
+  void saveDOM_Tree(const std::string &outFilename);
 
   /// Get the applied caching option.
   CachingOption getAppliedCachingOption() const;
@@ -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, const std::map<std::string, bool> &isTypeAssembly,
               std::map<std::string, Poco::XML::Element *> &getTypeElement);
 
   /// Take as input a \<locations\> element. Such an element is a short-hand
@@ -251,7 +251,7 @@ class MANTID_GEOMETRY_DLL InstrumentDefinitionParser {
 
   /// This method returns the parent appended which its child components and
   /// also name of type of the last child component
-  std::string getShapeCoorSysComp(Geometry::ICompAssembly *parent, Poco::XML::Element *pLocElem,
+  std::string getShapeCoorSysComp(Geometry::ICompAssembly *parent, const Poco::XML::Element *pLocElem,
                                   std::map<std::string, Poco::XML::Element *> &getTypeElement,
                                   Geometry::ICompAssembly *&endAssembly);
 

Framework/Geometry/inc/MantidGeometry/Instrument/XMLInstrumentParameter.h

@@ -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(const Mantid::Kernel::TimeSeriesProperty<double> *logData, const Kernel::TimeROI *) const;
 
   /// when this const equals 1 it means that angle=degree (default) is set in
   /// IDF

Framework/Geometry/src/Instrument/InstrumentDefinitionParser.cpp

@@ -590,7 +590,7 @@ void InstrumentDefinitionParser::appendLocations(Geometry::ICompAssembly *parent
  *
  *  @param outFilename :: Output filename
  */
-void InstrumentDefinitionParser::saveDOM_Tree(std::string &outFilename) {
+void InstrumentDefinitionParser::saveDOM_Tree(const std::string &outFilename) {
   Poco::XML::DOMWriter writer;
   writer.setNewLine("\n");
   writer.setOptions(Poco::XML::XMLWriter::PRETTY_PRINT);
@@ -1320,7 +1320,7 @@ void InstrumentDefinitionParser::createGridDetector(Geometry::ICompAssembly *par
                                                     const Poco::XML::Element *pCompElem, const std::string &filename,
                                                     const Poco::XML::Element *pType) {
 
-  //-------------- Create a RectangularDetector
+  //-------------- Create a GridDetector
   //------------------------------------------------
   std::string name = InstrumentDefinitionParser::getNameOfLocationElement(pLocElem, pCompElem);
 
@@ -1545,12 +1545,6 @@ void InstrumentDefinitionParser::createStructuredDetector(Geometry::ICompAssembl
   std::vector<double> xValues;
   std::vector<double> yValues;
 
-  // The shape!
-  // Given that this leaf component is actually an assembly, its constituent
-  // component detector shapes comes from its type attribute.
-  const std::string shapeType = pType->getAttribute("type");
-  std::shared_ptr<Geometry::IObject> shape = mapTypeNameToShape[shapeType];
-
   std::string typeName = pType->getAttribute("name");
   // These parameters are in the TYPE defining StructuredDetector
   if (pType->hasAttribute("xpixels"))
@@ -2327,7 +2321,7 @@ void InstrumentDefinitionParser::setLogfile(const Geometry::IComponent *comp, co
       unsigned long numberPoint = pNLpoint->length();
 
       for (unsigned long j = 0; j < numberPoint; j++) {
-        auto *pPoint = static_cast<Element *>(pNLpoint->item(j));
+        const auto *pPoint = static_cast<Element *>(pNLpoint->item(j));
         double x = attrToDouble(pPoint, "x");
         double y = attrToDouble(pPoint, "y");
         interpolation->addPoint(x, y);
@@ -2637,7 +2631,7 @@ void InstrumentDefinitionParser::createNeutronicInstrument() {
  *  @throw InstrumentDefinitionError Thrown if issues with the content of XML
  * instrument file
  */
-void InstrumentDefinitionParser::adjust(Poco::XML::Element *pElem, std::map<std::string, bool> &isTypeAssembly,
+void InstrumentDefinitionParser::adjust(Poco::XML::Element *pElem, const std::map<std::string, bool> &isTypeAssembly,
                                         std::map<std::string, Poco::XML::Element *> &getTypeElement) {
   UNUSED_ARG(isTypeAssembly)
   // check if pElem is an element with tag name 'type'
@@ -2674,7 +2668,7 @@ void InstrumentDefinitionParser::adjust(Poco::XML::Element *pElem, std::map<std:
 
   // check if a <translate-rotate-combined-shape-to> is defined
   Poco::AutoPtr<NodeList> pNL_TransRot = pElem->getElementsByTagName("translate-rotate-combined-shape-to");
-  Element *pTransRot = nullptr;
+  const Element *pTransRot = nullptr;
   if (pNL_TransRot->length() == 1) {
     pTransRot = static_cast<Element *>(pNL_TransRot->item(0));
   }
@@ -3056,7 +3050,7 @@ according to pLocElem
 instrument file
 */
 std::string InstrumentDefinitionParser::getShapeCoorSysComp(Geometry::ICompAssembly *parent,
-                                                            Poco::XML::Element *pLocElem,
+                                                            const Poco::XML::Element *pLocElem,
                                                             std::map<std::string, Poco::XML::Element *> &getTypeElement,
                                                             Geometry::ICompAssembly *&endAssembly) {
   // The location element is required to be a child of a component element.
@@ -3085,7 +3079,7 @@ std::string InstrumentDefinitionParser::getShapeCoorSysComp(Geometry::ICompAssem
   if (pNL->length() == 0) {
     return pType->getAttribute("name");
   } else if (pNL->length() == 1) {
-    auto *pElem = static_cast<Element *>(pNL->item(0));
+    const auto *pElem = static_cast<Element *>(pNL->item(0));
     return getShapeCoorSysComp(ass, pElem, getTypeElement, endAssembly);
   } else {
     throw Exception::InstrumentDefinitionError(

Framework/Geometry/src/Instrument/ObjCompAssembly.cpp

@@ -343,7 +343,7 @@ void ObjCompAssembly::testIntersectionWithChildren(Track &testRay,
       searchQueue.emplace_back(comp);
     }
     // Check the physical object intersection
-    else if (auto *physicalObject = dynamic_cast<IObjComponent *>(comp.get())) {
+    else if (const auto *physicalObject = dynamic_cast<IObjComponent *>(comp.get())) {
       physicalObject->interceptSurface(testRay);
     } else {
     }

Framework/Geometry/src/Instrument/XMLInstrumentParameter.cpp

@@ -18,6 +18,8 @@
 #include <boost/regex.hpp>
 #include <ctime>
 #include <fstream>
+#include <string>
+#include <string_view>
 #include <utility>
 
 namespace Mantid::Geometry {
@@ -89,7 +91,8 @@ XMLInstrumentParameter::XMLInstrumentParameter(std::string logfileID, std::strin
  *  @throw InstrumentDefinitionError Thrown if issues with the content of XML
  *instrument definition file
  */
-double XMLInstrumentParameter::createParamValue(TimeSeriesProperty<double> *logData, const Kernel::TimeROI *roi) const {
+double XMLInstrumentParameter::createParamValue(const TimeSeriesProperty<double> *logData,
+                                                const Kernel::TimeROI *roi) const {
   // If this parameter is a <look-up-table> or <formula> return 0.0. Such
   // parameter types are
   // associated with 'fitting' parameters. In some sense this method should
@@ -143,7 +146,7 @@ double XMLInstrumentParameter::createParamValue(TimeSeriesProperty<double> *logD
     }
     // Looking for string: "position n", where n is an integer and is a 1-based
     // index
-    else if (m_extractSingleValueAs.find("position") == 0 && m_extractSingleValueAs.size() >= 10) {
+    else if (m_extractSingleValueAs.starts_with("position") && m_extractSingleValueAs.size() >= 10) {
       std::stringstream extractPosition(m_extractSingleValueAs);
       std::string dummy;
       int position;

Framework/Geometry/src/Math/Acomp.cpp

@@ -162,7 +162,7 @@ Order (low first)
     if (*ax != *ux)
       return (*ux < *ax);
   }
-  return uc != Units.end();
+  return false;
 }
 
 Acomp &Acomp::operator+=(const Acomp &A)
@@ -490,9 +490,8 @@ It requires that the Intersect is the same for both
     return -1;
 
   if (!A.Units.empty()) {
-    auto Ept = Units.end();
     Units.resize(Units.size() + A.Units.size());
-    copy(A.Units.begin(), A.Units.end(), Ept);
+    copy(A.Units.begin(), A.Units.end(), back_inserter(Units));
     std::sort(Units.begin(), Units.end());
   }
 

Framework/Geometry/src/Objects/CSGObject.cpp

@@ -140,6 +140,8 @@ double coneSolidAngle(const V3D &observer, const Mantid::Kernel::V3D &centre, co
   double z0(0.0), z1(z_step);
   double r0(radius), r1(r0 - r_step);
 
+  // cppcheck-suppress knownConditionTrueFalse as although Cone::g_NSTACKS is currently set at 1 if this changes this
+  // code block would stop working
   for (int st = 1; st < Cone::g_NSTACKS; ++st) {
     for (int sl = 0; sl < Cone::g_NSLICES; ++sl) {
       int vertex = sl;
@@ -317,6 +319,8 @@ double cylinderSolidAngle(const V3D &observer, const V3D &centre, const V3D &axi
   double z0(0.0), z1(z_step);
   double solid_angle(0.0);
   for (int st = 1; st <= Cylinder::g_NSTACKS; ++st) {
+    // cppcheck-suppress knownConditionTrueFalse as although Cylinder::g_NSTACKS is currently set at 1 if this changes
+    // this code block is necessary
     if (st == Cylinder::g_NSTACKS)
       z1 = height;
 

buildconfig/CMake/CppCheck_Suppressions.txt.in

@@ -513,19 +513,6 @@ constVariablePointer:${CMAKE_SOURCE_DIR}/Framework/Geometry/src/Instrument/CompA
 constVariablePointer:${CMAKE_SOURCE_DIR}/Framework/Geometry/src/Instrument/CompAssembly.cpp:405
 constParameterPointer:${CMAKE_SOURCE_DIR}/Framework/Geometry/src/Instrument/ComponentInfo.cpp:129
 knownConditionTrueFalse:${CMAKE_SOURCE_DIR}/Framework/Geometry/src/Instrument/ComponentInfoBankHelpers.cpp:113
-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
 constVariableReference:${CMAKE_SOURCE_DIR}/Framework/Geometry/src/Rendering/RenderingHelpersOpenGL.cpp:204
 constParameterPointer:${CMAKE_SOURCE_DIR}/Framework/Geometry/src/Rendering/vtkGeometryCacheReader.cpp:64
 constParameterPointer:${CMAKE_SOURCE_DIR}/Framework/Geometry/src/Rendering/vtkGeometryCacheWriter.cpp:83