From 75ceb7dd8f9c77fdd45290a69baf27f9250baaf6 Mon Sep 17 00:00:00 2001
From: "Addisu Z. Taddese" <addisu@openrobotics.org>
Date: Thu, 30 Mar 2023 21:06:56 -0500
Subject: [PATCH] Suggestions for deprecating kSdfScopeDelimiter (#1265)

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
---
 include/sdf/Types.hh | 19 ++++++++++++++-----
 src/Types.cc         | 26 +++++++++++++++++---------
 src/Utils.cc         |  2 +-
 3 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/include/sdf/Types.hh b/include/sdf/Types.hh
index 28d5921be..f2f19248c 100644
--- a/include/sdf/Types.hh
+++ b/include/sdf/Types.hh
@@ -36,12 +36,21 @@ namespace sdf
   inline namespace SDF_VERSION_NAMESPACE {
   //
 
-  static const std::string &SdfScopeDelimiter()
+  namespace internal
   {
-    static const gz::utils::NeverDestroyed<std::string> delimiter{"::"};
-    return delimiter.Access();
-  }
-  static const std::string &kSdfScopeDelimiter = SdfScopeDelimiter();
+    /// \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/Types.cc b/src/Types.cc
index 60604bffc..8c3ddb5e9 100644
--- a/src/Types.cc
+++ b/src/Types.cc
@@ -96,12 +96,12 @@ 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};
@@ -109,20 +109,20 @@ std::pair<std::string, std::string> SplitName(
 
 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
index 6adbdc669..0097b0ed1 100644
--- 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);
     }