[Merge] lp:~phablet-team/media-hub/add-logger into lp:media-hub

[Alfonso Sanchez-Beato]

Review: Needs Fixing

I have some concerns with this MP:

* I do not really like moving to the printf way of doing things. Why should be drop type safeness? It would be better to use the boost logger more "natively" with "<<" operators, changing the wrapper class to something similar to what Qt does (qDebug() << message, etc.). Even more, using printf way in the end we need to resort to using stringstream in several places where it was not needed before.

* I still see cout and cerr being used in many places. But please do not change that yet until we have an agreement on how the class Logger should be used.

* I do not think we should introduce functions in the Utils namespace that we do not use. Specially taking into account that I would prefer to avoid using the only one that seems to be used, Sprintf :)

* Line sizes in new files are > 100 in many cases

There are also a couple of inline comments.


Diff comments:

> 
> === added file 'src/core/media/logger/logger.h'
> --- src/core/media/logger/logger.h	1970-01-01 00:00:00 +0000
> +++ src/core/media/logger/logger.h	2016-04-05 19:34:08 +0000
> @@ -0,0 +1,131 @@
> +/*
> + * Copyright (C) 2015 Canonical, Ltd.
> + *
> + * This program is free software: you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 3, as published
> + * by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranties of
> + * MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
> + * PURPOSE.  See the GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program.  If not, see <http://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#ifndef LOGGER_H_
> +#define LOGGER_H_
> +
> +#include "core/media/non_copyable.h"
> +#include "core/media/util/utils.h"
> +
> +#include <boost/optional.hpp>
> +
> +#include <string>
> +
> +namespace core {
> +namespace ubuntu {
> +namespace media {
> +// A Logger enables persisting of messages describing & explaining the
> +// state of the system.
> +class Logger : public core::ubuntu::media::NonCopyable {
> +public:
> +    // Severity enumerates all known severity levels
> +    // applicable to log messages.
> +    enum class Severity {
> +        kTrace,
> +        kDebug,
> +        kInfo,
> +        kWarning,
> +        kError,
> +        kFatal
> +    };

Maybe we could remove kTrace? Seems like too many levels and I do not see a clear difference with kDebug.

> +
> +    // A Location describes the origin of a log message.
> +    struct Location {
> +        std::string file; // The name of the file that contains the log message.
> +        std::string function; // The function that contains the log message.
> +        std::uint32_t line; // The line in file that resulted in the log message.
> +    };
> +
> +    virtual void Init(const core::ubuntu::media::Logger::Severity &severity = core::ubuntu::media::Logger::Severity::kWarning) = 0;
> +
> +    virtual void Log(Severity severity, const std::string &message, const boost::optional<Location>& location) = 0;
> +
> +    virtual void Trace(const std::string& message, const boost::optional<Location>& location = boost::optional<Location>{});
> +    virtual void Debug(const std::string& message, const boost::optional<Location>& location = boost::optional<Location>{});
> +    virtual void Info(const std::string& message, const boost::optional<Location>& location = boost::optional<Location>{});
> +    virtual void Warning(const std::string& message, const boost::optional<Location>& location = boost::optional<Location>{});
> +    virtual void Error(const std::string& message, const boost::optional<Location>& location = boost::optional<Location>{});
> +    virtual void Fatal(const std::string& message, const boost::optional<Location>& location = boost::optional<Location>{});
> +
> +
> +    template<typename... T>
> +    void Tracef(const boost::optional<Location>& location, const std::string& pattern, T&&...args) {
> +        Trace(Utils::Sprintf(pattern, std::forward<T>(args)...), location);
> +    }
> +
> +    template<typename... T>
> +    void Debugf(const boost::optional<Location>& location, const std::string& pattern, T&&...args) {
> +        Debug(Utils::Sprintf(pattern, std::forward<T>(args)...), location);
> +    }
> +
> +    template<typename... T>
> +    void Infof(const boost::optional<Location>& location, const std::string& pattern, T&&...args) {
> +        Info(Utils::Sprintf(pattern, std::forward<T>(args)...), location);
> +    }
> +
> +    template<typename... T>
> +    void Warningf(const boost::optional<Location>& location, const std::string& pattern, T&&...args) {
> +        Warning(Utils::Sprintf(pattern, std::forward<T>(args)...), location);
> +    }
> +
> +    template<typename... T>
> +    void Errorf(const boost::optional<Location>& location, const std::string& pattern, T&&...args) {
> +        Error(Utils::Sprintf(pattern, std::forward<T>(args)...), location);
> +    }
> +
> +    template<typename... T>
> +    void Fatalf(const boost::optional<Location>& location, const std::string& pattern, T&&...args) {
> +        Fatal(Utils::Sprintf(pattern, std::forward<T>(args)...), location);
> +    }
> +
> +protected:
> +    Logger() = default;
> +};
> +
> +// operator<< inserts severity into out.
> +std::ostream& operator<<(std::ostream& out, Logger::Severity severity);
> +
> +// operator<< inserts location into out.
> +std::ostream& operator<<(std::ostream& out, const Logger::Location &location);
> +
> +// Log returns the core::ubuntu::media-wide configured logger instance.
> +// Save to call before/after main.
> +Logger& Log();
> +// SetLog installs the given logger as core::ubuntu::media-wide default logger.
> +void SetLogger(const std::shared_ptr<Logger>& logger);
> +
> +#define TRACE(...) Log().Tracef(Logger::Location{__FILE__, __FUNCTION__, __LINE__}, __VA_ARGS__)
> +#define DEBUG(...) Log().Debugf(Logger::Location{__FILE__, __FUNCTION__, __LINE__}, __VA_ARGS__)
> +#define INFO(...) Log().Infof(Logger::Location{__FILE__, __FUNCTION__, __LINE__}, __VA_ARGS__)
> +#define WARNING(...) Log().Warningf(Logger::Location{__FILE__, __FUNCTION__, __LINE__}, __VA_ARGS__)
> +#define ERROR(...) Log().Errorf(Logger::Location{__FILE__, __FUNCTION__, __LINE__}, __VA_ARGS__)
> +#define FATAL(...) Log().Fatalf(Logger::Location{__FILE__, __FUNCTION__, __LINE__}, __VA_ARGS__)
> +} // namespace media
> +} // namespace ubuntu
> +} // namespace core
> +
> +#define MH_TRACE(...) core::ubuntu::media::Log().Tracef(\
> +        core::ubuntu::media::Logger::Location{__FILE__, __FUNCTION__, __LINE__}, __VA_ARGS__)

Why do we have TRACE and MH_TRACE? Why don't we use TRACE as macro name with the current definition of MH_TRACE?

> +#define MH_DEBUG(...) core::ubuntu::media::Log().Debugf(\
> +        core::ubuntu::media::Logger::Location{__FILE__, __FUNCTION__, __LINE__}, __VA_ARGS__)
> +#define MH_INFO(...) core::ubuntu::media::Log().Infof(\
> +        core::ubuntu::media::Logger::Location{__FILE__, __FUNCTION__, __LINE__}, __VA_ARGS__)
> +#define MH_WARNING(...) core::ubuntu::media::Log().Warningf(core::ubuntu::media::Logger::Location{__FILE__, __FUNCTION__, __LINE__}, __VA_ARGS__)
> +#define MH_ERROR(...) core::ubuntu::media::Log().Errorf(core::ubuntu::media::Logger::Location{__FILE__, __FUNCTION__, __LINE__}, __VA_ARGS__)
> +#define MH_FATAL(...) core::ubuntu::media::Log().Fatalf(core::ubuntu::media::Logger::Location{__FILE__, __FUNCTION__, __LINE__}, __VA_ARGS__)
> +
> +#endif


-- 
https://code.launchpad.net/~phablet-team/media-hub/add-logger/+merge/291010
Your team Ubuntu Phablet Team is subscribed to branch lp:media-hub.