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

[apps] Use sync.h instead of atomic.h in the verbose module. #3055

Merged

Conversation

maxsharabayko
Copy link
Collaborator

@maxsharabayko maxsharabayko commented Oct 21, 2024

The atomic.h header of the SRT project can include sync.h header in the following case:

#if defined(ATOMIC_USE_SRT_SYNC_MUTEX) && (ATOMIC_USE_SRT_SYNC_MUTEX == 1)
   #include "sync.h"
#endif

It leads to the compilation error because sync.h includes atomic.h and later atomic_clock.h:

[ 60%] Building CXX object /apps/socketoptions.cpp.o
In file included from /srtcore/sync.h:1069,
                 from /srt/srtcore/atomic.h:104,
                 from /srt/apps/verbose.hpp:15,
                 from /srt/apps/socketoptions.cpp:12:
/srt/srtcore/atomic_clock.h:25:5: error: 'atomic' does not name a type; did you mean 'atoi'?
     atomic<int64_t> dur;
     ^~~~~~
     atoi
/srt/srtcore/atomic_clock.h: In constructor 'srt::sync::AtomicDuration<Clock>::AtomicDuration()':
/srt/srtcore/atomic_clock.h:30:37: error: class 'srt::sync::AtomicDuration<Clock>' does not have any field named 'dur'
     AtomicDuration() ATR_NOEXCEPT : dur(0) {}
                                     ^~~
/submodule/srt/srtcore/atomic_clock.h: In member function 'srt::sync::AtomicDuration<Clock>::duration_type srt::sync::AtomicDuration<Clock>::load()':
/srt/srtcore/atomic_clock.h:34:23: error: 'dur' was not declared in this scope
         int64_t val = dur.load();
                       ^~~
/srt/srtcore/atomic_clock.h:34:23: note: suggested alternative: 'dup'
         int64_t val = dur.load();
                       ^~~
                       dup

The verbose module uses srt::sync::atomic<bool> lockline, so makes sense to include sync.h straight away.

Affected SRT versions: 1.5.4 RC.0, RC.1 (after PR #3004).

@maxsharabayko maxsharabayko added Type: Bug Indicates an unexpected problem or unintended behavior [apps] Area: Test applications related improvements labels Oct 21, 2024
@maxsharabayko maxsharabayko added this to the v1.5.4 milestone Oct 21, 2024
@maxsharabayko
Copy link
Collaborator Author

maxsharabayko commented Oct 24, 2024

Now when including verbose.hpp from a different app (e.g. xtransmit) the ENABLE_STDCXX_SYNC has to be properly defined in xtransmit as well 😞
Well, most of the time this should not be a problem.

@maxsharabayko
Copy link
Collaborator Author

maxsharabayko commented Oct 24, 2024

What about just using <atomic>? 🤔
There are common things referring to atomic, which can't use just any atomic. There are some things referring to formatting and locking for printing, especially in verbose, and conflicts may arise based on that.

@maxsharabayko maxsharabayko merged commit 7df2805 into Haivision:master Oct 29, 2024
12 checks passed
@maxsharabayko maxsharabayko deleted the hotfix/atomic-in-verbose branch October 29, 2024 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[apps] Area: Test applications related improvements Type: Bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant