Fix GSG violations on non-trivially destructible types (#1264)

* Types.hh, kSdfScopeDelimiter Signed-off-by: Aaron Chong <[email protected]> * use constexpr char for preferred_separator Signed-off-by: Aaron Chong <[email protected]> * parser_urdf, constexpr instead of const Signed-off-by: Aaron Chong <[email protected]> * ParticleEmitter, kEmitterTypeStrs Signed-off-by: Aaron Chong <[email protected]> * Camera, kPixelFormatNames Signed-off-by: Aaron Chong <[email protected]> * Sensor, kSensorTypeStrs Signed-off-by: Aaron Chong <[email protected]> * Types, kSdfScopeDelimiter, use string_view instead of char array Signed-off-by: Aaron Chong <[email protected]> * Revert changes to parser_urdf.cc Signed-off-by: Aaron Chong <[email protected]> * Added comment about size template parameter Signed-off-by: Aaron Chong <[email protected]> * Reverting mistake on double back slashes, and keeping static const char Signed-off-by: Aaron Chong <[email protected]> * Using NeverDestroyed for kSdfScopeDelimiter Signed-off-by: Aaron Chong <[email protected]> * Revert use of string_view for Sensor and Camera as macOS doesn't treat them as literals Signed-off-by: Aaron Chong <[email protected]> * Reverting to static const to hopefully appease win and macos Signed-off-by: Aaron Chong <[email protected]> * Fix implicit instantiate Signed-off-by: Aaron Chong <[email protected]> * Using const string_view this time Signed-off-by: Aaron Chong <[email protected]> * Suggestions for deprecating kSdfScopeDelimiter (#1265) Signed-off-by: Addisu Z. Taddese <[email protected]> --------- Signed-off-by: Aaron Chong <[email protected]> Signed-off-by: Addisu Z. Taddese <[email protected]> Co-authored-by: Addisu Z. Taddese <[email protected]>
gazebosim · Mar 31, 2023 · e98fcd8 · e98fcd8
1 parent 8a26cb4
commit e98fcd8
Show file tree

Hide file tree

Showing 9 changed files with 65 additions and 35 deletions.
diff --git a/include/sdf/ParticleEmitter.hh b/include/sdf/ParticleEmitter.hh
@@ -37,7 +37,7 @@ namespace sdf
 
   /// \enum ParticleEmitterType
   /// \brief The set of particle emitter types.
-  // Developer note: Make sure to update emitterTypeStrs in the source
+  // Developer note: Make sure to update kEmitterTypeStrs in the source
   // file when changing this enum.
   enum class ParticleEmitterType
   {

diff --git a/include/sdf/Sensor.hh b/include/sdf/Sensor.hh
@@ -49,7 +49,7 @@ namespace sdf
 
   /// \enum SensorType
   /// \brief The set of sensor types.
-  // Developer note: Make sure to update sensorTypeStrs in the source file
+  // Developer note: Make sure to update kSensorTypeStrs in the source file
   // when changing this enum.
   enum class SensorType
   {

diff --git a/include/sdf/Types.hh b/include/sdf/Types.hh
@@ -25,6 +25,7 @@
 #include <utility>
 #include <vector>
 
+#include <gz/utils/NeverDestroyed.hh>
 #include <sdf/sdf_config.h>
 #include "sdf/system_util.hh"
 #include "sdf/Error.hh"
@@ -35,7 +36,21 @@ namespace sdf
   inline namespace SDF_VERSION_NAMESPACE {
   //
 
-  const std::string kSdfScopeDelimiter = "::";
+  namespace internal
+  {
+    /// \brief Initializes the scope delimiter as a function-local static
+    /// variable so it can be used to initialize kSdfScopeDelimiter.
+    /// \note This should not be used directly in user code. It will likely be
+    /// removed in libsdformat 15 with kSdfScopeDelimiter.
+    SDFORMAT_VISIBLE const std::string &SdfScopeDelimiter();
+  }  // namespace internal
+
+  constexpr std::string_view kScopeDelimiter{"::"};
+
+  // Deprecated because it violates the Google Style Guide as it is not
+  // trivially destructible. Please use sdf::kScopeDelimiter instead.
+  GZ_DEPRECATED(14)
+  inline const std::string &kSdfScopeDelimiter = internal::SdfScopeDelimiter();
 
   /// \brief The source path replacement if it was parsed from a string,
   /// instead of a file.

diff --git a/src/Camera.cc b/src/Camera.cc
@@ -15,6 +15,8 @@
  *
 */
 #include <array>
+#include <string_view>
+
 #include "sdf/Camera.hh"
 #include "sdf/parser.hh"
 #include "Utils.hh"
@@ -23,7 +25,7 @@ using namespace sdf;
 
 /// \brief String names for the pixel formats.
 /// \sa Image::PixelFormat.
-static std::array<std::string, 19> kPixelFormatNames =
+constexpr std::array<const std::string_view, 19> kPixelFormatNames =
 {
   "UNKNOWN_PIXEL_FORMAT",
   "L_INT8",
@@ -1146,9 +1148,9 @@ std::string Camera::ConvertPixelFormat(PixelFormatType _type)
 {
   unsigned int index = static_cast<int>(_type);
   if (index < kPixelFormatNames.size())
-    return kPixelFormatNames[static_cast<int>(_type)];
+    return std::string(kPixelFormatNames[static_cast<int>(_type)]);
 
-  return kPixelFormatNames[0];
+  return std::string(kPixelFormatNames[0]);
 }
 
 /////////////////////////////////////////////////

diff --git a/src/Filesystem.cc b/src/Filesystem.cc
@@ -90,7 +90,7 @@ class DirIter::Implementation
 
 #ifndef _WIN32
 
-static const char preferred_separator = '/';
+static const char kSdfFileSystemPreferredSeparator = '/';
 
 //////////////////////////////////////////////////
 bool exists(const std::string &_path)
@@ -210,7 +210,7 @@ void DirIter::close_handle()
 
 #else  // Windows
 
-static const char preferred_separator = '\\';
+static const char kSdfFileSystemPreferredSeparator = '\\';
 
 //////////////////////////////////////////////////
 static bool not_found_error(int _errval)
@@ -496,7 +496,7 @@ void DirIter::close_handle()
 //////////////////////////////////////////////////
 const std::string separator(const std::string &_p)
 {
-  return _p + preferred_separator;
+  return _p + kSdfFileSystemPreferredSeparator;
 }
 
 //////////////////////////////////////////////////
@@ -509,7 +509,7 @@ std::string basename(const std::string &_path)
 
   for (size_t i = 0; i < _path.length(); ++i)
   {
-    if (_path[i] == preferred_separator)
+    if (_path[i] == kSdfFileSystemPreferredSeparator)
     {
       if (i == (_path.length() - 1))
       {
@@ -519,7 +519,7 @@ std::string basename(const std::string &_path)
         // basename is empty, we return just a "/".
         if (basename.size() == 0)
         {
-          basename.push_back(preferred_separator);
+          basename.push_back(kSdfFileSystemPreferredSeparator);
         }
         break;
       }
@@ -556,7 +556,7 @@ DirIter::DirIter() : dataPtr(gz::utils::MakeUniqueImpl<Implementation>())
 //////////////////////////////////////////////////
 std::string DirIter::operator*() const
 {
-  return this->dataPtr->dirname + preferred_separator +
+  return this->dataPtr->dirname + kSdfFileSystemPreferredSeparator +
     this->dataPtr->current;
 }
 

diff --git a/src/ParticleEmitter.cc b/src/ParticleEmitter.cc
@@ -14,10 +14,11 @@
  * limitations under the License.
  *
 */
+#include <array>
 #include <memory>
 #include <optional>
 #include <string>
-#include <vector>
+#include <string_view>
 
 #include "sdf/Error.hh"
 #include "sdf/parser.hh"
@@ -31,8 +32,9 @@
 using namespace sdf;
 
 /// Particle emitter type strings. These should match the data in
-/// `enum class ParticleEmitterType` located in ParticleEmitter.hh.
-const std::vector<std::string> emitterTypeStrs =
+/// `enum class ParticleEmitterType` located in ParticleEmitter.hh, and the size
+/// template parameter should match the number of elements as well.
+constexpr std::array<const std::string_view, 4> kEmitterTypeStrs =
 {
   "point",
   "box",
@@ -250,9 +252,9 @@ void ParticleEmitter::SetType(const ParticleEmitterType _type)
 /////////////////////////////////////////////////
 bool ParticleEmitter::SetType(const std::string &_typeStr)
 {
-  for (size_t i = 0; i < emitterTypeStrs.size(); ++i)
+  for (size_t i = 0; i < kEmitterTypeStrs.size(); ++i)
   {
-    if (_typeStr == emitterTypeStrs[i])
+    if (_typeStr == kEmitterTypeStrs[i])
     {
       this->dataPtr->type = static_cast<ParticleEmitterType>(i);
       return true;
@@ -265,8 +267,8 @@ bool ParticleEmitter::SetType(const std::string &_typeStr)
 std::string ParticleEmitter::TypeStr() const
 {
   size_t index = static_cast<int>(this->dataPtr->type);
-  if (index < emitterTypeStrs.size())
-    return emitterTypeStrs[index];
+  if (index < kEmitterTypeStrs.size())
+    return std::string(kEmitterTypeStrs[index]);
   return "point";
 }
 

diff --git a/src/Sensor.cc b/src/Sensor.cc
@@ -14,8 +14,10 @@
  * limitations under the License.
  *
 */
+#include <array>
 #include <memory>
 #include <string>
+#include <string_view>
 #include <optional>
 #include <vector>
 #include <gz/math/Pose3.hh>
@@ -39,8 +41,9 @@
 using namespace sdf;
 
 /// Sensor type strings. These should match the data in
-/// `enum class SensorType` located in Sensor.hh.
-const std::vector<std::string> sensorTypeStrs =
+/// `enum class SensorType` located in Sensor.hh, and the size template
+/// parameter should match the number of elements as well.
+constexpr std::array<const std::string_view, 27> kSensorTypeStrs =
 {
   "none",
   "altimeter",
@@ -528,9 +531,9 @@ void Sensor::SetType(const SensorType _type)
 /////////////////////////////////////////////////
 bool Sensor::SetType(const std::string &_typeStr)
 {
-  for (size_t i = 0; i < sensorTypeStrs.size(); ++i)
+  for (size_t i = 0; i < kSensorTypeStrs.size(); ++i)
   {
-    if (_typeStr == sensorTypeStrs[i])
+    if (_typeStr == kSensorTypeStrs[i])
     {
       this->dataPtr->type = static_cast<SensorType>(i);
       return true;
@@ -645,8 +648,8 @@ void Sensor::SetUpdateRate(double _hz)
 std::string Sensor::TypeStr() const
 {
   size_t index = static_cast<int>(this->dataPtr->type);
-  if (index > 0 && index < sensorTypeStrs.size())
-    return sensorTypeStrs[index];
+  if (index > 0 && index < kSensorTypeStrs.size())
+    return std::string(kSensorTypeStrs[index]);
   return "none";
 }
 

diff --git a/src/Types.cc b/src/Types.cc
@@ -96,33 +96,33 @@ std::ostream &operator<<(std::ostream &_out, const sdf::Errors &_errs)
 std::pair<std::string, std::string> SplitName(
     const std::string &_absoluteName)
 {
-  const auto pos = _absoluteName.rfind(kSdfScopeDelimiter);
+  const auto pos = _absoluteName.rfind(kScopeDelimiter);
   if (pos != std::string::npos)
   {
     const std::string first = _absoluteName.substr(0, pos);
     const std::string second =
-        _absoluteName.substr(pos + kSdfScopeDelimiter.size());
+        _absoluteName.substr(pos + kScopeDelimiter.size());
     return {first, second};
   }
   return {"", _absoluteName};
 }
 
 static bool EndsWithDelimiter(const std::string &_s)
 {
-  if (_s.size() < kSdfScopeDelimiter.size())
+  if (_s.size() < kScopeDelimiter.size())
     return false;
 
-  const size_t startPosition = _s.size() - kSdfScopeDelimiter.size();
+  const size_t startPosition = _s.size() - kScopeDelimiter.size();
   return _s.compare(
-    startPosition, kSdfScopeDelimiter.size(), kSdfScopeDelimiter) == 0;
+    startPosition, kScopeDelimiter.size(), kScopeDelimiter) == 0;
 }
 
 static bool StartsWithDelimiter(const std::string &_s)
 {
-  if (_s.size() < kSdfScopeDelimiter.size())
+  if (_s.size() < kScopeDelimiter.size())
     return false;
 
-  return _s.compare(0, kSdfScopeDelimiter.size(), kSdfScopeDelimiter) == 0;
+  return _s.compare(0, kScopeDelimiter.size(), kScopeDelimiter) == 0;
 }
 
 // Join a scope name prefix with a local name using the scope delimeter
@@ -138,11 +138,19 @@ std::string JoinName(
   const bool localNameStartsWithDelimiter = StartsWithDelimiter(_localName);
 
   if (scopeNameEndsWithDelimiter && localNameStartsWithDelimiter)
-    return _scopeName + _localName.substr(kSdfScopeDelimiter.size());
+    return _scopeName + _localName.substr(kScopeDelimiter.size());
   else if (scopeNameEndsWithDelimiter || localNameStartsWithDelimiter)
     return _scopeName + _localName;
   else
-    return _scopeName + kSdfScopeDelimiter + _localName;
+    return _scopeName + std::string(kScopeDelimiter) + _localName;
+}
+
+/////////////////////////////////////////////////
+const std::string &internal::SdfScopeDelimiter()
+{
+  static const gz::utils::NeverDestroyed<std::string> delimiter{
+      std::string()};
+  return delimiter.Access();
 }
 }
 }
diff --git a/src/Utils.cc b/src/Utils.cc
@@ -207,7 +207,7 @@ static std::optional<std::string> computeAbsoluteName(
     std::advance(it, 1);
     for (; it != names.rend(); ++it)
     {
-      absoluteParentName.append(kSdfScopeDelimiter);
+      absoluteParentName.append(kScopeDelimiter);
       absoluteParentName.append(*it);
     }