Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not include Boost.Log related headers when MQTT_USE_LOG is off #927

Merged
merged 5 commits into from
Apr 9, 2022
Merged

Do not include Boost.Log related headers when MQTT_USE_LOG is off #927

merged 5 commits into from
Apr 9, 2022

Conversation

Tradias
Copy link
Contributor

@Tradias Tradias commented Apr 8, 2022

What

Avoid including headers that are only needed when MQTT_USE_LOG is enabled.

Why

Reduce the compile times and avoid unexpected linkage with Boost.Log libraries (e.g. when Boost auto-link is enabled).

Background

I am happy that mqtt_cpp provides such log customization option. I used it to adapt spdlog. The code for it here as a reference for other users:

mqttCpp.h:

#pragma once

#include <boost/preprocessor/cat.hpp>
#include <boost/preprocessor/comparison/greater_equal.hpp>
#include <boost/preprocessor/if.hpp>
#include <spdlog/common.h>

#include <source_location>
#include <sstream>
#include <string_view>

#define MQTT_NS mqtt_cpp

// forward declare mqtt_cpp severity_level
namespace mqtt_cpp
{
enum class severity_level;
}  // namespace mqtt_cpp

namespace my_app
{
struct MqttNullLogAdapter
{
};

template <class T>
constexpr MqttNullLogAdapter operator<<(MqttNullLogAdapter, const T&) noexcept
{
    return {};
}

struct MqttLogAdapter
{
    std::string_view channel;
    mqtt_cpp::severity_level severity;
    std::source_location location;
    std::ostringstream stream;

    explicit MqttLogAdapter(std::string_view channel, mqtt_cpp::severity_level severity,
                            std::source_location location = std::source_location::current())
        : channel(channel), severity(severity), location(location)
    {
    }

    ~MqttLogAdapter();

    template <class T>
    friend MqttLogAdapter&& operator<<(MqttLogAdapter&& self, const T& t);
};

struct MqttLogOstreamNotImplemented
{
};
}  // namespace my_app

#define MY_APP_MQTT_SEVERITY_SPDLOG_LEVEL_trace SPDLOG_LEVEL_TRACE
#define MY_APP_MQTT_SEVERITY_SPDLOG_LEVEL_debug SPDLOG_LEVEL_DEBUG
#define MY_APP_MQTT_SEVERITY_SPDLOG_LEVEL_info SPDLOG_LEVEL_INFO
#define MY_APP_MQTT_SEVERITY_SPDLOG_LEVEL_warning SPDLOG_LEVEL_WARN
#define MY_APP_MQTT_SEVERITY_SPDLOG_LEVEL_error SPDLOG_LEVEL_ERROR
#define MY_APP_MQTT_SEVERITY_SPDLOG_LEVEL_fatal SPDLOG_LEVEL_CRITICAL

#define MY_APP_MQTT_SEVERITY_TO_SPDLOG_LEVEL(lv) BOOST_PP_CAT(MY_APP_MQTT_SEVERITY_SPDLOG_LEVEL_, lv)

#define MQTT_LOG(chan, sev)                                                                              \
    BOOST_PP_IF(BOOST_PP_GREATER_EQUAL(MY_APP_MQTT_SEVERITY_TO_SPDLOG_LEVEL(sev), SPDLOG_ACTIVE_LEVEL), \
                ::my_app::MqttLogAdapter(chan, MQTT_NS::severity_level::sev),                     \
                ::my_app::MqttNullLogAdapter())

#define MQTT_ADD_VALUE(name, val) ::my_app::MqttLogOstreamNotImplemented()

// Add mqtt_cpp library includes here
#include <mqtt_client_cpp.hpp>

namespace my_app
{
template <class T>
MqttLogAdapter&& operator<<(MqttLogAdapter&& self, const T& t)
{
    if constexpr (!std::is_same_v<mqtt::MqttLogOstreamNotImplemented, T>)
    {
        self.stream << t;
    }
    return std::move(self);
}
}  // namespace my_app

mqttCpp.cpp

#include "mqttCpp.h"

#include <spdlog/spdlog.h>

namespace my_app
{
MqttLogAdapter::~MqttLogAdapter()
{
    const auto spdlog_level = [&]
    {
        if (mqtt_cpp::severity_level::trace == severity)
        {
            return spdlog::level::trace;
        }
        if (mqtt_cpp::severity_level::debug == severity)
        {
            return spdlog::level::debug;
        }
        if (mqtt_cpp::severity_level::info == severity)
        {
            return spdlog::level::info;
        }
        if (mqtt_cpp::severity_level::warning == severity)
        {
            return spdlog::level::warn;
        }
        if (mqtt_cpp::severity_level::error == severity)
        {
            return spdlog::level::err;
        }
        if (mqtt_cpp::severity_level::fatal == severity)
        {
            return spdlog::level::critical;
        }
        return spdlog::level::off;
    }();
    spdlog::default_logger_raw()->log(
        spdlog::source_loc{location.file_name(), static_cast<int>(location.line()), location.function_name()},
        spdlog_level, "{}: {}", channel, stream.view());
}
}  // namespace my_app

@codecov
Copy link

codecov bot commented Apr 8, 2022

Codecov Report

Merging #927 (866d850) into master (ae1945a) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #927      +/-   ##
==========================================
- Coverage   83.08%   83.05%   -0.03%     
==========================================
  Files          64       64              
  Lines       10357    10357              
==========================================
- Hits         8605     8602       -3     
- Misses       1752     1755       +3     

@Tradias
Copy link
Contributor Author

Tradias commented Apr 8, 2022

@redboltz I do not have access to the code coverage report. Is there anything unexpected there?

Nevermind, seems like it took a moment for Codecov to load all results.

Copy link
Owner

@redboltz redboltz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please add comments.

@@ -24,12 +28,12 @@
#include <boost/preprocessor/cat.hpp>
#include <boost/preprocessor/comparison/greater_equal.hpp>

#endif
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#endif
#endif // defined(MQTT_USE_LOG)

Please add comment. See https://github.com/redboltz/mqtt_cpp/wiki/Coding-Rules#dont-use-negative-comparison-with-else

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am used to this being taken care of by an autoformatter, but I didn't see a .clang-format file. Is there a different formatter that you use?

Copy link
Owner

@redboltz redboltz Apr 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use my own emacs setting ;).
I tried to use clang-format and wrote my rule.
Here is it:

Language: Cpp

# Indent
IndentWidth: 4
AccessModifierOffset: -4
#IndentBraces: false
ConstructorInitializerIndentWidth: 4
ContinuationIndentWidth: 4
IndentAccessModifiers: false
IndentCaseBlocks: false
IndentCaseLabels: false
IndentExternBlock: NoIndent
AlignAfterOpenBracket: AlwaysBreak
AllowAllParametersOfDeclarationOnNextLine: true
IndentGotoLabels: false
IndentPPDirectives: None
IndentRequires: true
IndentWrappedFunctionNames: false
LambdaBodyIndentation: Signature
UseTab: Never

#namespace
#AfterNamespace: false
#SplitEmptyNamespace: false
CompactNamespaces: false
FixNamespaceComments: true

# Align
AlignAfterOpenBracket: BlockIndent
#AlignAfterOpenBracket: AlwaysBreak
AlignArrayOfStructures: Right
AlignConsecutiveAssignments: AcrossEmptyLinesAndComments
AlignConsecutiveBitFields: AcrossEmptyLinesAndComments
AlignConsecutiveDeclarations: AcrossEmptyLinesAndComments
AlignConsecutiveMacros: AcrossEmptyLinesAndComments
AlignEscapedNewlines: Left
AlignOperands: Align
AlignTrailingComments: true
PointerAlignment: Left
ReferenceAlignment: Left

# Break
AlwaysBreakBeforeMultilineStrings: true
AlwaysBreakTemplateDeclarations: Yes
PenaltyBreakTemplateDeclaration: 0
BinPackArguments: false
BinPackParameters: false

BreakBeforeBraces: Custom
BraceWrapping:
  AfterCaseLabel: false
  BeforeCatch: true
  BeforeElse: true
  BeforeLambdaBody: true
  BeforeWhile: false

BreakBeforeBinaryOperators: None
BreakBeforeConceptDeclarations: true
BreakBeforeTernaryOperators: true
BreakConstructorInitializers: BeforeColon
BreakInheritanceList: AfterColon
BreakStringLiterals: false

AllowAllConstructorInitializersOnNextLine: true
PackConstructorInitializers: Never



# SingleLine
AllowShortBlocksOnASingleLine: Empty
AllowShortCaseLabelsOnASingleLine: false
AllowShortEnumsOnASingleLine: false
AllowShortFunctionsOnASingleLine: Empty
AllowShortIfStatementsOnASingleLine: Never
AllowShortLambdasOnASingleLine: None
AllowShortLoopsOnASingleLine: false
#SplitEmptyFunction: true
#SplitEmptyRecord: true

#Macro

#Space
Cpp11BracedListStyle: false
DerivePointerAlignment: true
SpaceAfterCStyleCast: false
SpaceAfterLogicalNot: false
SpaceAfterTemplateKeyword: true
SpaceAroundPointerQualifiers: Default
SpaceBeforeAssignmentOperators: true
SpaceBeforeCaseColon: false
SpaceBeforeCpp11BracedList: false
SpaceBeforeCtorInitializerColon: false
SpaceBeforeInheritanceColon: true
SpaceBeforeParens: ControlStatements
SpaceBeforeRangeBasedForLoopColon: true
SpaceBeforeSquareBrackets: false
SpaceInEmptyBlock: false
SpaceInEmptyParentheses: false
SpacesBeforeTrailingComments: 1
SpacesInAngles: Never
SpacesInCStyleCastParentheses: false
SpacesInConditionalStatement: false
SpacesInParentheses: false
SpacesInSquareBrackets: false

# Other
FixNamespaceComments: true
IncludeBlocks: Preserve
MaxEmptyLinesToKeep: 2
SortIncludes: CaseSensitive
SortUsingDeclarations: true
AlignAfterOpenBracket: BlockIndent

might be error. In order to use is, I build clang-format master branch from the source code.

However, it is not enough for me.

I want to format like as follows:

void foo(
    int param1,
    char param2,
    double param3
) {
    async_function(
        arg1,
        arg2,
        [
            capture1,
            capture2 = std::move(outer),
            capture3
        ]
        () mutable -> std::size_t {
            // body
            ++capture1;
        }
    );
}

As far as I studied, the current clang-format doesn't provide much customization point for lamnda expression (especially capture list).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .clang-format above is not used for mqtt_cpp. Just demonstrate what I tried.

include/mqtt/setup_log.hpp Outdated Show resolved Hide resolved
@redboltz
Copy link
Owner

redboltz commented Apr 9, 2022

Thank you for updating.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants