The upd_sink and dist_sink files have been modified to address Passed by value warnings

[KaganCanSit]

Hello,

This pull request addresses CppCheck's Passed By Value warnings. Additional information on this topic was provided in the discussion section here, and unnecessary/misleading warnings were removed from the list.

Remaining Warnings;

include/spdlog/sinks/udp_sink.h:39:performance:passedByValue -> Function parameter ‘sink_config’ should be passed by const reference.
include/spdlog/sinks/dist_sink.h:26:performance:passedByValue -> Function parameter ‘sinks’ should be passed by const reference.

---

I'm not sure about the explicit constructor in dist_sink.h!

When vector<shared_ptr<...>> is copied here, each shared_ptr is copied, creating shared ownership of the relevant objects. In this case, ownership of the objects is shared between the caller and the objects.

If the caller provides an rvalue with std::move, the container and its elements are moved; the caller's argument becomes moved-from (valid but with undefined content). This does not guarantee absolute sole ownership of the managed objects; if there are other copies of shared_ptr, ownership is already shared.

Since this ctor stores the parameter internally, I passed it to the member using by-value + std::move to reduce unnecessary copying without changing the API. This approach does not move the caller's argument in lvalue calls; in rvalue calls, it allows direct transfer.

However, I'm a bit hesitant about this improvement. Since this forward-looking constructor is not currently used in the project, it could be removed, or if clearer behavior is desired, const& + && overload alternatives could be considered.

---

That said, I fixed warnings as much as possible outside of fmt and tests. If you notice anything I missed, please let me know via comment/ping; I'll address it when I can. I'll keep the Discussion topic open for a while in case it turns out to be a bug (as I mentioned earlier). (I'm thinking a maximum of 1 week.)

Thank you for your guidance.

Best regards.

[KaganCanSit]

This change affects the API.

[Copilot]

Pull request overview

This PR addresses CppCheck "Passed By Value" performance warnings in the udp_sink and dist_sink header files by optimizing parameter passing strategies.

Changes:

• Modified udp_sink constructor to accept udp_sink_config by const reference instead of by value
• Modified dist_sink constructor to use move semantics for the sinks vector parameter

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
include/spdlog/sinks/udp_sink.h	Changed constructor parameter from pass-by-value to pass-by-const-reference
include/spdlog/sinks/dist_sink.h	Added std::move to constructor initialization to enable move semantics

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The constructor parameter should be passed by value to enable the copy-and-swap idiom with std::move. However, for optimal flexibility, consider providing both const reference and rvalue reference overloads, or pass by const reference if move semantics are not intended. The current approach (pass-by-value + move) is valid but may not be the clearest API design without documentation explaining the ownership transfer semantics.

[gabime]

If the caller provides an rvalue with std::move, the container and its elements are moved; the caller's argument becomes moved-from (valid but with undefined content). This does not guarantee absolute sole ownership of the managed objects; if there are other copies of shared_ptr, ownership is already shared.

So whats the problem? this is the intended usage of move..

[KaganCanSit]

If the caller provides an rvalue with std::move, the container and its elements are moved; the caller's argument becomes moved-from (valid but with undefined content). This does not guarantee absolute sole ownership of the managed objects; if there are other copies of shared_ptr, ownership is already shared.

So whats the problem? this is the intended usage of move..

I may not have expressed myself clearly here. I adopted this approach so that there would be no change in behavior. The following examples and explanations will help you:

• Main Code (No Change): https://godbolt.org/z/49v8o1q57
• Move-Only Constructor (m_vec_container(std::move(vec_container)) /* No copying here */): https://godbolt.org/z/P9eeo57fo
• const & and std::move Change: https://godbolt.org/z/sh3EdEnje

With const&, even if the caller explicitly uses std::move() to indicate they want to transfer ownership, the move is silently ignored and a copy occurs instead. This violates the caller's expectations.

Value signing correctly reflects the caller's intent:

• Lvalue → copy (caller retains the data)
• Rvalue/std::move → move (caller discards the data)

My PR preserves this behavior. However, if your goal is to perform an operation without the user accessing the source code, you should use const &. I wanted to point this out since I wasn't aware of it.

[gabime]

Thanks @KaganCanSit

[KaganCanSit]

Thanks @KaganCanSit

You're welcome. If there are any errors or if you need help, you can ping me. I'd be happy to help when I'm available. It was nice to spend time on a different project.