Update Logger API in C++ (#3013)

cpp/include/Ice/Logger.h

@@ -22,6 +22,10 @@ namespace Ice
     public:
         virtual ~Logger() = default;
 
+        // We use const std::string& and not std::string_view for the log messages because implementations commonly
+        // send the message to C APIs that require null-terminated strings.
+        // The message itself is also often constructed from a string produced by an ostringstream.
+
         /**
          * Print a message. The message is printed literally, without any decorations such as executable name or time
          * stamp.
@@ -61,7 +65,7 @@ namespace Ice
          * @param prefix The new prefix for the logger.
          * @return A logger instance.
          */
-        virtual LoggerPtr cloneWithPrefix(const std::string& prefix) = 0;
+        virtual LoggerPtr cloneWithPrefix(std::string prefix) = 0;
     };
 }
 

cpp/include/Ice/StringConverter.h

@@ -148,7 +148,7 @@ namespace Ice
      * @param nc The narrow string converter. If null, the UTF-8 string is returned.
      * @return A native narrow string.
      */
-    ICE_API std::string UTF8ToNative(const std::string& str, const StringConverterPtr& nc);
+    ICE_API std::string UTF8ToNative(std::string_view str, const StringConverterPtr& nc);
 
 #ifdef _WIN32
 

cpp/include/IceBox/Service.h

@@ -1,21 +1,11 @@
-//
-// Copyright (c) ZeroC, Inc. All rights reserved.
-//
+// Copyright (c) ZeroC, Inc.
 
 #ifndef ICEBOX_SERVICE_H
 #define ICEBOX_SERVICE_H
 
 #include "Config.h"
 #include "Ice/Ice.h"
 
-#if defined(__clang__)
-#    pragma clang diagnostic push
-#    pragma clang diagnostic ignored "-Wshadow-field-in-constructor"
-#elif defined(__GNUC__)
-#    pragma GCC diagnostic push
-#    pragma GCC diagnostic ignored "-Wshadow"
-#endif
-
 namespace IceBox
 {
     /**
@@ -61,10 +51,4 @@ namespace IceBox
     using ServicePtr = std::shared_ptr<Service>;
 }
 
-#if defined(__clang__)
-#    pragma clang diagnostic pop
-#elif defined(__GNUC__)
-#    pragma GCC diagnostic pop
-#endif
-
 #endif

cpp/src/Ice/LoggerAdminI.cpp

@@ -111,7 +111,8 @@ namespace
         void warning(const std::string&) final;
         void error(const std::string&) final;
         std::string getPrefix() final;
-        LoggerPtr cloneWithPrefix(const std::string&) final;
+        LoggerPtr cloneWithPrefix(std::string) final;
+
         ObjectPtr getFacet() const;
         void destroy() final;
 
@@ -629,9 +630,9 @@ namespace
 
     string LoggerAdminLoggerI::getPrefix() { return _localLogger->getPrefix(); }
 
-    LoggerPtr LoggerAdminLoggerI::cloneWithPrefix(const string& prefix)
+    LoggerPtr LoggerAdminLoggerI::cloneWithPrefix(string prefix)
     {
-        return _localLogger->cloneWithPrefix(prefix);
+        return _localLogger->cloneWithPrefix(std::move(prefix));
     }
 
     ObjectPtr LoggerAdminLoggerI::getFacet() const { return _loggerAdmin; }

cpp/src/Ice/LoggerI.cpp

@@ -25,21 +25,21 @@ namespace
     const chrono::minutes retryTimeout = chrono::minutes(5);
 }
 
-Ice::LoggerI::LoggerI(const string& prefix, const string& file, bool convert, size_t sizeMax)
-    : _prefix(prefix),
+Ice::LoggerI::LoggerI(string prefix, string file, bool convert, size_t sizeMax)
+    : _prefix(std::move(prefix)),
       _convert(convert),
       _converter(getProcessStringConverter()),
+      _file(std::move(file)),
       _sizeMax(sizeMax)
 {
-    if (!prefix.empty())
+    if (!_prefix.empty())
     {
-        _formattedPrefix = prefix + ": ";
+        _formattedPrefix = _prefix + ": ";
     }
 
-    if (!file.empty())
+    if (!_file.empty())
     {
-        _file = file;
-        _out.open(IceInternal::streamFilename(file).c_str(), fstream::out | fstream::app);
+        _out.open(IceInternal::streamFilename(_file).c_str(), fstream::out | fstream::app);
         if (!_out.is_open())
         {
             throw InitializationException(__FILE__, __LINE__, "FileLogger: cannot open " + _file);
@@ -102,10 +102,10 @@ Ice::LoggerI::getPrefix()
 }
 
 LoggerPtr
-Ice::LoggerI::cloneWithPrefix(const std::string& prefix)
+Ice::LoggerI::cloneWithPrefix(std::string prefix)
 {
     lock_guard lock(outputMutex); // for _sizeMax
-    return make_shared<LoggerI>(prefix, _file, _convert, _sizeMax);
+    return make_shared<LoggerI>(std::move(prefix), _file, _convert, _sizeMax);
 }
 
 void

cpp/src/Ice/LoggerI.h

@@ -12,18 +12,18 @@
 
 namespace Ice
 {
-    class LoggerI : public Logger
+    class LoggerI final : public Logger
     {
     public:
-        LoggerI(const std::string&, const std::string&, bool convert = true, std::size_t sizeMax = 0);
+        LoggerI(std::string prefix, std::string file, bool convert = true, std::size_t sizeMax = 0);
         ~LoggerI();
 
-        virtual void print(const std::string&);
-        virtual void trace(const std::string&, const std::string&);
-        virtual void warning(const std::string&);
-        virtual void error(const std::string&);
-        virtual std::string getPrefix();
-        virtual LoggerPtr cloneWithPrefix(const std::string&);
+        void print(const std::string&) final;
+        void trace(const std::string& category, const std::string& message) final;
+        void warning(const std::string&) final;
+        void error(const std::string&) final;
+        std::string getPrefix() final;
+        LoggerPtr cloneWithPrefix(std::string) final;
 
     private:
         void write(const std::string&, bool);
@@ -34,7 +34,7 @@ namespace Ice
         const StringConverterPtr _converter;
         std::ofstream _out;
 
-        std::string _file;
+        const std::string _file;
         std::size_t _sizeMax;
 
         //

cpp/src/Ice/OSLogLoggerI.cpp

@@ -10,48 +10,48 @@
 using namespace std;
 using namespace Ice;
 
-Ice::OSLogLoggerI::OSLogLoggerI(const std::string& prefix) : _prefix(prefix)
+Ice::OSLogLoggerI::OSLogLoggerI(string prefix)
+    : _prefix(std::move(prefix)),
+      _subsystem(_prefix.empty() ? "com.zeroc.ice" : "com.zeroc.ice." + _prefix),
+      _log(os_log_create(_subsystem.c_str(), ""))
 {
-    const string subsystem = prefix.empty() ? "com.zeroc.ice" : "com.zeroc.ice." + prefix;
-    _log.reset(os_log_create(subsystem.c_str(), ""));
 }
 
 void
-Ice::OSLogLoggerI::print(const std::string& message)
+Ice::OSLogLoggerI::print(const string& message)
 {
     os_log_with_type(_log.get(), OS_LOG_TYPE_DEFAULT, "%{public}s.", message.c_str());
 }
 
 void
-Ice::OSLogLoggerI::trace(const std::string& category, const std::string& message)
+Ice::OSLogLoggerI::trace(const string& category, const string& message)
 {
-    const string subsystem = _prefix.empty() ? "com.zeroc.ice" : "com.zeroc.ice." + _prefix;
-    IceInternal::UniqueRef<os_log_t> log(os_log_create(subsystem.c_str(), category.c_str()));
+    IceInternal::UniqueRef<os_log_t> log(os_log_create(_subsystem.c_str(), category.c_str()));
     os_log_with_type(log.get(), OS_LOG_TYPE_INFO, "%{public}s.", message.c_str());
 }
 
 void
-Ice::OSLogLoggerI::warning(const std::string& message)
+Ice::OSLogLoggerI::warning(const string& message)
 {
     os_log_with_type(_log.get(), OS_LOG_TYPE_ERROR, "%{public}s.", message.c_str());
 }
 
 void
-Ice::OSLogLoggerI::error(const std::string& message)
+Ice::OSLogLoggerI::error(const string& message)
 {
     os_log_with_type(_log.get(), OS_LOG_TYPE_FAULT, "%{public}s.", message.c_str());
 }
 
-std::string
+string
 Ice::OSLogLoggerI::getPrefix()
 {
     return _prefix;
 }
 
 LoggerPtr
-Ice::OSLogLoggerI::cloneWithPrefix(const std::string& prefix)
+Ice::OSLogLoggerI::cloneWithPrefix(string prefix)
 {
-    return make_shared<OSLogLoggerI>(prefix);
+    return make_shared<OSLogLoggerI>(std::move(prefix));
 }
 
 #endif

cpp/src/Ice/OSLogLoggerI.h

@@ -13,20 +13,21 @@
 
 namespace Ice
 {
-    class OSLogLoggerI : public Logger
+    class OSLogLoggerI final : public Logger
     {
     public:
-        OSLogLoggerI(const std::string&);
+        OSLogLoggerI(std::string);
 
-        virtual void print(const std::string&);
-        virtual void trace(const std::string&, const std::string&);
-        virtual void warning(const std::string&);
-        virtual void error(const std::string&);
-        virtual std::string getPrefix();
-        virtual LoggerPtr cloneWithPrefix(const std::string&);
+        void print(const std::string&) final;
+        void trace(const std::string&, const std::string&) final;
+        void warning(const std::string&) final;
+        void error(const std::string&) final;
+        std::string getPrefix() final;
+        LoggerPtr cloneWithPrefix(std::string) final;
 
     private:
         const std::string _prefix;
+        const std::string _subsystem;
         IceInternal::UniqueRef<os_log_t> _log;
     };
 }

cpp/src/Ice/Service.cpp

@@ -117,27 +117,29 @@ namespace
     };
     using SMEventLoggerPtr = std::shared_ptr<SMEventLogger>;
 
-    class SMEventLoggerIWrapper : public Ice::Logger
+    class SMEventLoggerIWrapper final : public Ice::Logger
     {
     public:
-        SMEventLoggerIWrapper(const SMEventLoggerPtr& logger, const string& prefix) : _logger(logger), _prefix(prefix)
+        SMEventLoggerIWrapper(const SMEventLoggerPtr& logger, string prefix)
+            : _logger(logger),
+              _prefix(std::move(prefix))
         {
             assert(_logger);
         }
 
-        virtual void print(const string& message) { _logger->print(_prefix, message); }
+        void print(const string& message) final { _logger->print(_prefix, message); }
 
-        void trace(const string& category, const string& message) { _logger->trace(_prefix, category, message); }
+        void trace(const string& category, const string& message) final { _logger->trace(_prefix, category, message); }
 
-        virtual void warning(const string& message) { _logger->warning(_prefix, message); }
+        void warning(const string& message) final { _logger->warning(_prefix, message); }
 
-        virtual void error(const string& message) { _logger->error(_prefix, message); }
+        void error(const string& message) final { _logger->error(_prefix, message); }
 
-        virtual string getPrefix() { return _prefix; }
+        string getPrefix() final { return _prefix; }
 
-        virtual Ice::LoggerPtr cloneWithPrefix(const string& prefix)
+        Ice::LoggerPtr cloneWithPrefix(string prefix) final
         {
-            return make_shared<SMEventLoggerIWrapper>(_logger, prefix);
+            return make_shared<SMEventLoggerIWrapper>(_logger, std::move(prefix));
         }
 
     private:

cpp/src/Ice/StringConverter.cpp

@@ -341,11 +341,11 @@ Ice::nativeToUTF8(string_view str, const Ice::StringConverterPtr& converter)
 }
 
 string
-Ice::UTF8ToNative(const string& str, const Ice::StringConverterPtr& converter)
+Ice::UTF8ToNative(string_view str, const Ice::StringConverterPtr& converter)
 {
     if (!converter || str.empty())
     {
-        return str;
+        return string{str};
     }
     string tmp;
     converter->fromUTF8(

cpp/src/Ice/SysLoggerI.cpp

@@ -12,7 +12,7 @@ using namespace std;
 using namespace Ice;
 using namespace IceInternal;
 
-Ice::SysLoggerI::SysLoggerI(const string& prefix, const string& facilityString) : _facility(0), _prefix(prefix)
+Ice::SysLoggerI::SysLoggerI(string prefix, string_view facilityString) : _facility(0), _prefix(std::move(prefix))
 {
     if (facilityString == "LOG_KERN")
     {
@@ -100,14 +100,17 @@ Ice::SysLoggerI::SysLoggerI(const string& prefix, const string& facilityString)
     }
     else
     {
-        throw InitializationException(__FILE__, __LINE__, "Invalid value for Ice.SyslogFacility: " + facilityString);
+        throw InitializationException(
+            __FILE__,
+            __LINE__,
+            "Invalid value for Ice.SyslogFacility: " + string{facilityString});
     }
 
     int logopt = LOG_PID | LOG_CONS;
     openlog(_prefix.c_str(), logopt, _facility);
 }
 
-Ice::SysLoggerI::SysLoggerI(const string& prefix, int facility) : _facility(facility), _prefix(prefix)
+Ice::SysLoggerI::SysLoggerI(string prefix, int facility) : _facility(facility), _prefix(std::move(prefix))
 {
     int logopt = LOG_PID | LOG_CONS;
     openlog(_prefix.c_str(), logopt, facility);
@@ -151,9 +154,9 @@ Ice::SysLoggerI::getPrefix()
 }
 
 Ice::LoggerPtr
-Ice::SysLoggerI::cloneWithPrefix(const string& prefix)
+Ice::SysLoggerI::cloneWithPrefix(string prefix)
 {
-    return make_shared<SysLoggerI>(prefix, _facility);
+    return make_shared<SysLoggerI>(std::move(prefix), _facility);
 }
 
 #endif

cpp/src/Ice/SysLoggerI.h

@@ -8,22 +8,23 @@
 #include "Ice/Logger.h"
 
 #include <mutex>
+#include <string_view>
 
 namespace Ice
 {
-    class SysLoggerI : public Logger
+    class SysLoggerI final : public Logger
     {
     public:
-        SysLoggerI(const std::string&, const std::string&);
-        SysLoggerI(const std::string&, int);
+        SysLoggerI(std::string prefix, std::string_view facilityString);
+        SysLoggerI(std::string prefix, int facility);
         ~SysLoggerI();
 
-        virtual void print(const std::string&);
-        virtual void trace(const std::string&, const std::string&);
-        virtual void warning(const std::string&);
-        virtual void error(const std::string&);
-        virtual std::string getPrefix();
-        virtual LoggerPtr cloneWithPrefix(const std::string&);
+        void print(const std::string&) final;
+        void trace(const std::string&, const std::string&) final;
+        void warning(const std::string&) final;
+        void error(const std::string&) final;
+        std::string getPrefix() final;
+        LoggerPtr cloneWithPrefix(std::string) final;
 
     private:
         int _facility;