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

ethouris · 2024-09-03T13:11:44Z

Changes are added for various reasons.

Inline comments have been added to explain the reason of particular changes.

ethouris · 2024-09-03T13:14:36Z

.github/workflows/cxx11-win.yaml

@@ -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


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

ethouris · 2024-09-03T13:15:01Z

.github/workflows/cxx11-win.yaml

    - name: build
-      run: cd _build && cmake --build ./ --config Release
+      run: cd _build && cmake --build ./ --config Release --verbose


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

ethouris · 2024-09-03T13:15:18Z

srtcore/congctl.cpp

@@ -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 << ")");


Precision is 6 by default.

ethouris · 2024-09-03T13:15:48Z

srtcore/handshake.cpp

@@ -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" };


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

Maybe something for 1.5.4. 🤔

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".

ethouris · 2024-09-03T13:16:48Z

srtcore/logging.h

@@ -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); } \


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

ethouris · 2024-09-03T13:18:51Z

srtcore/logging.h

-        // 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)
+        size_t your_pfx_len = your_pfx ? strlen(your_pfx) : 0;


This avoids using snprintf and playing with alternatives. The length of the strings is taken once and then all copying is done on memcpy with known size. The snprintf function was prone to various warnings either through clang or MSVC.

ethouris · 2024-09-03T13:19:23Z

srtcore/logging.h

@@ -381,7 +382,7 @@ struct LogDispatcher::Proxy
            buf[len-1] = '\0';
        }

-        os << buf;
+        os.write(buf, len);


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

ethouris · 2024-09-03T13:19:41Z

srtcore/queue.h

+        , m_iPort(0)
+        , m_iIPversion(0)
+        , m_iRefCount(1)
+        , m_iID(-1)


Somehow these values have never been initialized. Since UDT.

ethouris · 2024-09-03T13:20:58Z

srtcore/utilities.h

    using namespace std;

    ostringstream os;
+    os << setfill('0') << setw(2) << hex << uppercase;


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.

ethouris · 2024-09-03T13:22:49Z

testing/srt-test-multiplex.cpp

@@ -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)) {}


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.

ethouris · 2024-09-03T13:23:21Z

testing/testactivemedia.cpp

@@ -120,4 +120,20 @@ void TargetMedium::Runner()
    }
 }

+bool TargetMedium::Schedule(const MediaPacket& data)


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

ethouris · 2024-09-03T13:24:11Z

testing/testactivemedia.hpp

@@ -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};


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.

srtcore/logging.h

Added suggestions from code review. Co-authored-by: Maxim Sharabayko <maxlovic@gmail.com>

[core] Some refactoring per clang-reported warnings

6d7e44e

ethouris commented Sep 3, 2024

View reviewed changes

maxsharabayko reviewed Sep 3, 2024

View reviewed changes

srtcore/logging.h Outdated Show resolved Hide resolved

srtcore/logging.h Outdated Show resolved Hide resolved

Apply suggestions from code review

5d4227d

Added suggestions from code review. Co-authored-by: Maxim Sharabayko <maxlovic@gmail.com>

ethouris added this to the v1.5.4 milestone Sep 4, 2024

ethouris added Type: Maintenance Work required to maintain or clean up the code [apps] Area: Test applications related improvements [core] Area: Changes in SRT library core [build] Area: Changes in build files labels Sep 9, 2024

maxsharabayko merged commit ff96e17 into Haivision:master Sep 9, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ethouris commented Sep 3, 2024 •

edited

Loading

ethouris Sep 3, 2024

ethouris Sep 3, 2024

ethouris Sep 3, 2024

ethouris Sep 3, 2024

maxsharabayko Sep 3, 2024

ethouris Sep 3, 2024

ethouris Sep 3, 2024

ethouris Sep 3, 2024

ethouris Sep 3, 2024

ethouris Sep 3, 2024

ethouris Sep 3, 2024

ethouris Sep 3, 2024

ethouris Sep 3, 2024

ethouris Sep 3, 2024

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

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

Conversation

ethouris commented Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethouris commented Sep 3, 2024 •

edited

Loading