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

[core][apps] Some refactoring per clang-reported warnings #3019

Merged
merged 2 commits into from
Sep 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/cxx11-win.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ jobs:
- name: configure
run: |
md _build && cd _build
cmake ../ -DENABLE_STDCXX_SYNC=ON -DENABLE_ENCRYPTION=OFF -DENABLE_UNITTESTS=ON -DENABLE_BONDING=ON
cmake ../ -DENABLE_STDCXX_SYNC=ON -DENABLE_ENCRYPTION=OFF -DENABLE_UNITTESTS=ON -DENABLE_BONDING=ON -DUSE_CXX_STD=c++11
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is to prevent the compiler from compiling in any other version than exactly C++11.

- name: build
run: cd _build && cmake --build ./ --config Release
run: cd _build && cmake --build ./ --config Release --verbose
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

CI builds need to be verbose, otherwise any error will be unclear.

- name: test
run: cd _build && ctest -E "TestIPv6.v6_calls_v4|TestConnectionTimeout.BlockingLoop" --extra-verbose -C Release
2 changes: 1 addition & 1 deletion srtcore/congctl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -595,7 +595,7 @@ class FileCC : public SrtCongestionControlBase
{
m_dPktSndPeriod = m_dCWndSize / (m_parent->SRTT() + m_iRCInterval);
HLOGC(cclog.Debug, log << "FileCC: CHKTIMER, SLOWSTART:OFF, sndperiod=" << m_dPktSndPeriod << "us AS wndsize/(RTT+RCIV) (wndsize="
<< setprecision(6) << m_dCWndSize << " RTT=" << m_parent->SRTT() << " RCIV=" << m_iRCInterval << ")");
<< m_dCWndSize << " RTT=" << m_parent->SRTT() << " RCIV=" << m_iRCInterval << ")");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Precision is 6 by default.

}
}
else
Expand Down
2 changes: 1 addition & 1 deletion srtcore/handshake.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ std::string srt::SrtFlagString(int32_t flags)
#define LEN(arr) (sizeof (arr)/(sizeof ((arr)[0])))

std::string output;
static std::string namera[] = { "TSBPD-snd", "TSBPD-rcv", "haicrypt", "TLPktDrop", "NAKReport", "ReXmitFlag", "StreamAPI" };
static std::string namera[] = { "TSBPD-snd", "TSBPD-rcv", "haicrypt", "TLPktDrop", "NAKReport", "ReXmitFlag", "StreamAPI", "FilterCapable" };
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This field wasn't added properly after adding a corresponding flag.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe something for 1.5.4. 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was considering it. All other changes don't disturb anyway.

This one of course isn't a crash, at worst it will report this flag as "unknown".


size_t i = 0;
for (; i < LEN(namera); ++i)
Expand Down
51 changes: 26 additions & 25 deletions srtcore/logging.h
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ written by
{ \
srt_logging::LogDispatcher::Proxy log(logdes); \
log.setloc(__FILE__, __LINE__, __FUNCTION__); \
const srt_logging::LogDispatcher::Proxy& log_prox SRT_ATR_UNUSED = args; \
{ (void)(const srt_logging::LogDispatcher::Proxy&)(args); } \
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Despite the unused attribute, this may generate a warning in some situations, depending on what is in args. This is safer.

}

// LOGF uses printf-like style formatting.
Expand Down Expand Up @@ -147,6 +147,7 @@ struct SRT_API LogDispatcher
LogLevel::type level;
static const size_t MAX_PREFIX_SIZE = 32;
char prefix[MAX_PREFIX_SIZE+1];
size_t prefix_len;
LogConfig* src_config;

bool isset(int flg) { return (src_config->flags & flg) != 0; }
Expand All @@ -159,30 +160,30 @@ struct SRT_API LogDispatcher
level(log_level),
src_config(&config)
{
// XXX stpcpy desired, but not enough portable
// Composing the exact prefix is not critical, so simply
// cut the prefix, if the length is exceeded

// See Logger::Logger; we know this has normally 2 characters,
// except !!FATAL!!, which has 9. Still less than 32.
// If the size of the FA name together with severity exceeds the size,
// just skip the former.
if (logger_pfx && strlen(prefix) + strlen(logger_pfx) + 1 < MAX_PREFIX_SIZE)
const size_t your_pfx_len = your_pfx ? strlen(your_pfx) : 0;
const size_t logger_pfx_len = logger_pfx ? strlen(logger_pfx) : 0;

if (logger_pfx && your_pfx_len + logger_pfx_len + 1 < MAX_PREFIX_SIZE)
{
#if defined(_MSC_VER) && _MSC_VER < 1900
_snprintf(prefix, MAX_PREFIX_SIZE, "%s:%s", your_pfx, logger_pfx);
#else
snprintf(prefix, MAX_PREFIX_SIZE + 1, "%s:%s", your_pfx, logger_pfx);
#endif
memcpy(prefix, your_pfx, your_pfx_len);
prefix[your_pfx_len] = ':';
memcpy(prefix + your_pfx_len + 1, logger_pfx, logger_pfx_len);
prefix[your_pfx_len + logger_pfx_len + 1] = '\0';
prefix_len = your_pfx_len + logger_pfx_len + 1;
}
else if (your_pfx)
{
// Prefix too long, so copy only your_pfx and only
// as much as it fits
size_t copylen = std::min(+MAX_PREFIX_SIZE, your_pfx_len);
memcpy(prefix, your_pfx, copylen);
prefix[copylen] = '\0';
prefix_len = copylen;
}
else
{
#ifdef _MSC_VER
strncpy_s(prefix, MAX_PREFIX_SIZE + 1, your_pfx, _TRUNCATE);
#else
strncpy(prefix, your_pfx, MAX_PREFIX_SIZE);
prefix[MAX_PREFIX_SIZE] = '\0';
#endif
prefix[0] = '\0';
prefix_len = 0;
}
}

Expand Down Expand Up @@ -338,9 +339,9 @@ struct LogDispatcher::Proxy

~Proxy()
{
if ( that_enabled )
if (that_enabled)
{
if ( (flags & SRT_LOGF_DISABLE_EOL) == 0 )
if ((flags & SRT_LOGF_DISABLE_EOL) == 0)
os << std::endl;
that.SendLogLine(i_file, i_line, area, os.str());
}
Expand Down Expand Up @@ -381,7 +382,7 @@ struct LogDispatcher::Proxy
buf[len-1] = '\0';
}

os << buf;
os.write(buf, len);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is done to bypass any formatting features of ostringstream (idem the next one).

return *this;
}
};
Expand Down Expand Up @@ -494,7 +495,7 @@ inline void LogDispatcher::SendLogLine(const char* file, int line, const std::st
}
else if ( src_config->log_stream )
{
(*src_config->log_stream) << msg;
src_config->log_stream->write(msg.data(), msg.size());
(*src_config->log_stream).flush();
}
src_config->unlock();
Expand Down
4 changes: 4 additions & 0 deletions srtcore/queue.h
Original file line number Diff line number Diff line change
Expand Up @@ -591,6 +591,10 @@ struct CMultiplexer
, m_pRcvQueue(NULL)
, m_pChannel(NULL)
, m_pTimer(NULL)
, m_iPort(0)
, m_iIPversion(0)
, m_iRefCount(1)
, m_iID(-1)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Somehow these values have never been initialized. Since UDT.

{
}

Expand Down
21 changes: 2 additions & 19 deletions srtcore/utilities.h
Original file line number Diff line number Diff line change
Expand Up @@ -981,30 +981,13 @@ inline std::string FormatBinaryString(const uint8_t* bytes, size_t size)
if ( size == 0 )
return "";

//char buf[256];
using namespace std;

ostringstream os;
os << setfill('0') << setw(2) << hex << uppercase;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That old version with so much of messing around was unncessary. Even if the state clearing after a single value was in plans before C++98, it was finally decided that the flags are changed permanently by manipultors.


// I know, it's funny to use sprintf and ostringstream simultaneously,
// but " %02X" in iostream is: << " " << hex << uppercase << setw(2) << setfill('0') << VALUE << setw(1)
// Too noisy. OTOH ostringstream solves the problem of memory allocation
// for a string of unpredictable size.
//sprintf(buf, "%02X", int(bytes[0]));

os.fill('0');
os.width(2);
os.setf(ios::basefield, ios::hex);
os.setf(ios::uppercase);

//os << buf;
os << int(bytes[0]);


for (size_t i = 1; i < size; ++i)
for (size_t i = 0; i < size; ++i)
{
//sprintf(buf, " %02X", int(bytes[i]));
//os << buf;
os << int(bytes[i]);
}
return os.str();
Expand Down
2 changes: 1 addition & 1 deletion testing/srt-test-live.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ extern "C" int SrtCheckGroupHook(void* , SRTSOCKET acpsock, int , const sockaddr
size = sizeof gt;
if (-1 != srt_getsockflag(acpsock, SRTO_GROUPTYPE, &gt, &size))
{
if (gt < Size(gtypes))
if (size_t(gt) < Size(gtypes))
Verb(" type=", gtypes[gt], VerbNoEOL);
else
Verb(" type=", int(gt), VerbNoEOL);
Expand Down
8 changes: 4 additions & 4 deletions testing/srt-test-multiplex.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ struct MediumPair
bytevector initial_portion;
string name;

MediumPair(unique_ptr<Source> s, unique_ptr<Target> t): src(move(s)), tar(move(t)) {}
MediumPair(unique_ptr<Source> s, unique_ptr<Target> t): src(std::move(s)), tar(std::move(t)) {}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This and below: by some reason clang strictly requires that move be always used as std::move explicitly. Probably because this is a library support for a language feature and could be easily confused if the possibly local move name is in use.


void Stop()
{
Expand Down Expand Up @@ -190,9 +190,9 @@ class MediaBase
/// are still meant to be delivered to @c tar
MediumPair& Link(std::unique_ptr<Source> src, std::unique_ptr<Target> tar, bytevector&& initial_portion, string name, string thread_name)
{
media.emplace_back(move(src), move(tar));
media.emplace_back(std::move(src), std::move(tar));
MediumPair& med = media.back();
med.initial_portion = move(initial_portion);
med.initial_portion = std::move(initial_portion);
med.name = name;

// Ok, got this, so we can start transmission.
Expand Down Expand Up @@ -382,7 +382,7 @@ bool SelectAndLink(SrtModel& m, string id, bool mode_output, string& w_msg)
}

bytevector dummy_initial_portion;
g_media_base.Link(move(source), move(target), move(dummy_initial_portion), os.str(), thread_name);
g_media_base.Link(std::move(source), std::move(target), std::move(dummy_initial_portion), os.str(), thread_name);

return true;
}
Expand Down
4 changes: 2 additions & 2 deletions testing/srt-test-relay.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,7 @@ SrtMainLoop::SrtMainLoop(const string& srt_uri, bool input_echoback, const strin
Verb() << "Setting up output: " << spec;
unique_ptr<TargetMedium> m { new TargetMedium };
m->Setup(Target::Create(spec));
m_output_media.push_back(move(m));
m_output_media.push_back(std::move(m));
}


Expand Down Expand Up @@ -369,7 +369,7 @@ SrtMainLoop::SrtMainLoop(const string& srt_uri, bool input_echoback, const strin
// Add SRT medium to output targets, and keep input medium empty.
unique_ptr<TargetMedium> med { new TargetMedium };
med->Setup(m_srt_relay.get());
m_output_media.push_back(move(med));
m_output_media.push_back(std::move(med));
}
else
{
Expand Down
16 changes: 16 additions & 0 deletions testing/testactivemedia.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -120,4 +120,20 @@ void TargetMedium::Runner()
}
}

bool TargetMedium::Schedule(const MediaPacket& data)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This function was moved here in order to not pollute the header file.

{
LOGP(applog.Debug, "TargetMedium::Schedule LOCK ... ");
std::lock_guard<std::mutex> lg(buffer_lock);
LOGP(applog.Debug, "TargetMedium::Schedule LOCKED - checking: running=", running, " interrupt=", ::transmit_int_state);
if (!running || ::transmit_int_state)
{
LOGP(applog.Debug, "TargetMedium::Schedule: not running, discarding packet");
return false;
}

LOGP(applog.Debug, "TargetMedium(", typeid(*med).name(), "): Schedule: [", data.payload.size(), "] CLIENT -> BUFFER");
buffer.push_back(data);
ready.notify_one();
return true;
}

20 changes: 2 additions & 18 deletions testing/testactivemedia.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
#include <exception>
#include <thread>
#include <mutex>
#include <atomic>
#include <condition_variable>

#include "testmedia.hpp"
Expand All @@ -29,7 +28,7 @@ struct Medium
std::mutex buffer_lock;
std::thread thr;
std::condition_variable ready;
std::atomic<bool> running = {false};
srt::sync::atomic<bool> running {false};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For consistency, in all SRT applications, which also use internal SRT facilities, the std::atomic should not be used and srt::sync::atomic is used instead.

std::exception_ptr xp; // To catch exception thrown by a thread

virtual void Runner() = 0;
Expand Down Expand Up @@ -147,22 +146,7 @@ struct TargetMedium: Medium<Target>
{
void Runner() override;

bool Schedule(const MediaPacket& data)
{
LOGP(applog.Debug, "TargetMedium::Schedule LOCK ... ");
std::lock_guard<std::mutex> lg(buffer_lock);
LOGP(applog.Debug, "TargetMedium::Schedule LOCKED - checking: running=", running, " interrupt=", ::transmit_int_state);
if (!running || ::transmit_int_state)
{
LOGP(applog.Debug, "TargetMedium::Schedule: not running, discarding packet");
return false;
}

LOGP(applog.Debug, "TargetMedium(", typeid(*med).name(), "): Schedule: [", data.payload.size(), "] CLIENT -> BUFFER");
buffer.push_back(data);
ready.notify_one();
return true;
}
bool Schedule(const MediaPacket& data);

void Clear()
{
Expand Down
9 changes: 4 additions & 5 deletions testing/testmedia.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
#include <map>
#include <chrono>
#include <thread>
#include <atomic>
#include <srt.h>
#if !defined(_WIN32)
#include <sys/ioctl.h>
Expand Down Expand Up @@ -50,8 +49,8 @@ using srt_logging::SockStatusStr;
using srt_logging::MemberStatusStr;
#endif

std::atomic<bool> transmit_throw_on_interrupt {false};
std::atomic<bool> transmit_int_state {false};
srt::sync::atomic<bool> transmit_throw_on_interrupt {false};
srt::sync::atomic<bool> transmit_int_state {false};
int transmit_bw_report = 0;
unsigned transmit_stats_report = 0;
size_t transmit_chunk_size = SRT_LIVE_DEF_PLSIZE;
Expand Down Expand Up @@ -348,7 +347,7 @@ void SrtCommon::InitParameters(string host, string path, map<string,string> par)
}

cc.token = token++;
m_group_nodes.push_back(move(cc));
m_group_nodes.push_back(std::move(cc));
}

par.erase("type");
Expand Down Expand Up @@ -3042,7 +3041,7 @@ extern unique_ptr<Base> CreateMedium(const string& uri)
}

if (ptr)
ptr->uri = move(u);
ptr->uri = std::move(u);
return ptr;
}

Expand Down
5 changes: 3 additions & 2 deletions testing/testmedia.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@
#include <map>
#include <stdexcept>
#include <deque>
#include <atomic>

#include <sync.h> // use srt::sync::atomic instead of std::atomic for the sake of logging

#include "apputil.hpp"
#include "statswriter.hpp"
Expand All @@ -25,7 +26,7 @@

extern srt_listen_callback_fn* transmit_accept_hook_fn;
extern void* transmit_accept_hook_op;
extern std::atomic<bool> transmit_int_state;
extern srt::sync::atomic<bool> transmit_int_state;

extern std::shared_ptr<SrtStatsWriter> transmit_stats_writer;

Expand Down
4 changes: 2 additions & 2 deletions testing/testmediabase.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,17 @@
#ifndef INC_SRT_COMMON_TRANMITBASE_HPP
#define INC_SRT_COMMON_TRANMITBASE_HPP

#include <atomic>
#include <string>
#include <memory>
#include <vector>
#include <iostream>
#include <stdexcept>

#include "sync.h"
#include "uriparser.hpp"

typedef std::vector<char> bytevector;
extern std::atomic<bool> transmit_throw_on_interrupt;
extern srt::sync::atomic<bool> transmit_throw_on_interrupt;
extern int transmit_bw_report;
extern unsigned transmit_stats_report;
extern size_t transmit_chunk_size;
Expand Down
Loading