Skip to content

Add constructor for dup_filter_sink with sinks parameter#3549

Merged
gabime merged 2 commits intogabime:v1.xfrom
Niram7777:patch-1
Mar 14, 2026
Merged

Add constructor for dup_filter_sink with sinks parameter#3549
gabime merged 2 commits intogabime:v1.xfrom
Niram7777:patch-1

Conversation

@Niram7777
Copy link
Copy Markdown
Contributor

No description provided.

@Niram7777
Copy link
Copy Markdown
Contributor Author

Niram7777 commented Mar 1, 2026

for some strange reasons, I am still not able to add any sink at all to dup_filter_sink (my other sinks work)
I have tried pretty much everything...
image
image

@Niram7777
Copy link
Copy Markdown
Contributor Author

the issue also happens with just

        std::initializer_list<spdlog::sink_ptr> sinks{
            console_sink, file_sink,
            std::make_shared<
                spdlog::sinks::
                    null_sink_mt>() /*for POSITON_GAME_LOG id + ".log" */};
        spdlog::sinks::dist_sink<std::mutex>().set_sinks(sinks);

@tt4g
Copy link
Copy Markdown
Contributor

tt4g commented Mar 1, 2026

The cause is the usage of std::initializer_list.
Copying std::initializer_list does not copy its internal elements: https://stackoverflow.com/questions/27157202/can-it-be-safe-to-keep-a-copy-of-an-stdinitializer-list-what-is-the-rationale
Change sinks to a different container type (such as std::vector).

@Niram7777
Copy link
Copy Markdown
Contributor Author

Niram7777 commented Mar 1, 2026

The cause is the usage of std::initializer_list. Copying std::initializer_list does not copy its internal elements:

it was actually an issue with lldb 23, the vector size is correct with 19 - I am surprized with the rootcause but will report the issue to LLVM

my PR is ready

@tt4g
Copy link
Copy Markdown
Contributor

tt4g commented Mar 2, 2026

No, your code is incorrect.
As noted in the StackOverflow comment, the C++ standard explicitly states that "Copying an initializer list does not copy the underlying elements".

As you can see on the cppreference.com page for the std::initializer_list constructor, std::initializer_list does not have a copy constructor.
This means that copying the std:initializer_list is not defined by the C++ standard.

Applying the following changes to your code will make it work:

+        std::vector<spdlog::sink_ptr> sinks({
-        std::initializer_list<spdlog::sink_ptr> sinks{
             console_sink, file_sink,
             std::make_shared<
                 spdlog::sinks::
+                    null_sink_mt>() /*for POSITON_GAME_LOG id + ".log" */});
-                    null_sink_mt>() /*for POSITON_GAME_LOG id + ".log" */};
         spdlog::sinks::dist_sink<std::mutex>().set_sinks(sinks);

explicit dup_filter_sink(std::chrono::duration<Rep, Period> max_skip_duration, std::vector<std::shared_ptr<sink>> sinks)
: dist_sink<Mutex>(std::move(sinks))
, max_skip_duration_{max_skip_duration} {}

Copy link
Copy Markdown
Owner

@gabime gabime Mar 7, 2026

Choose a reason for hiding this comment

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

max_skip_duration should be initialized before the vector , to follow the declare order

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

https://en.cppreference.com/w/cpp/language/initializer_list.html

  1. Then, direct bases are initialized in left-to-right order as they appear in this class's base-specifier list.
  2. Then, non-static data member are initialized in order of declaration in the class definition.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Even so, the code should be clean as possible

@tt4g
Copy link
Copy Markdown
Contributor

tt4g commented Mar 12, 2026

@Niram7777 I think this change was created for shadps4-emu/shadPS4#4069, but since the previous suggestion resolved your issue, is this PR no longer needed?

@Niram7777
Copy link
Copy Markdown
Contributor Author

@Niram7777 I think this change was created for shadps4-emu/shadPS4#4069, but since the previous suggestion resolved your issue, is this PR no longer needed?

sorry I have almost no free time during the week, and I am super busy on the weekend.
I am not forgetting this PR although I though it was trivial, I will look at it this weekend

@Niram7777
Copy link
Copy Markdown
Contributor Author

No, your code is incorrect. As noted in the StackOverflow comment, the C++ standard explicitly states that "Copying an initializer list does not copy the underlying elements".

As you can see on the cppreference.com page for the std::initializer_list constructor, std::initializer_list does not have a copy constructor. This means that copying the std:initializer_list is not defined by the C++ standard.

Applying the following changes to your code will make it work:

+        std::vector<spdlog::sink_ptr> sinks({
-        std::initializer_list<spdlog::sink_ptr> sinks{
             console_sink, file_sink,
             std::make_shared<
                 spdlog::sinks::
+                    null_sink_mt>() /*for POSITON_GAME_LOG id + ".log" */});
-                    null_sink_mt>() /*for POSITON_GAME_LOG id + ".log" */};
         spdlog::sinks::dist_sink<std::mutex>().set_sinks(sinks);

"Copying an initializer list does not copy the underlying elements".

yes and what the standard says is that it will be a shallow copy -> so the representation will be copied, so the size should not be 0

std::initializer_list does not copy the backing array of the corresponding initializer list.

std::initializer_list<int> il{-3, -2, -1};
assert(il.begin()[2] == -1); // note the replacement for absent operator[]
il = al; // shallow-copy
assert(il.begin() == al.begin()); // guaranteed

also

A std::initializer_list object is automatically constructed when:
a brace-enclosed initializer list is used as the right operand of assignment or as a function call argument, and the corresponding assignment operator/function accepts an std::initializer_list parameter,

https://en.cppreference.com/w/cpp/container/vector/vector.html

vector( std::initializer_list init,
const Allocator& alloc = Allocator() );
...
11) Equivalent to vector(il.begin(), il.end(), alloc).

@gabime gabime merged commit 45b67ee into gabime:v1.x Mar 14, 2026
1 check passed
@gabime
Copy link
Copy Markdown
Owner

gabime commented Mar 14, 2026

Thanks @Niram7777

@Niram7777 Niram7777 deleted the patch-1 branch March 15, 2026 21:42
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.

3 participants