Disabled warnings various platforms and fixed C++20 Windows build #2411

oviano · 2022-07-20T07:25:59Z

adds some more disabled warnings to platform_sys.h on Apple platforms, Windows and Android.
include platform_sys.h in some haicrypt files to remove warnings from these files too.
removed unary_function when compiling C++20, as it was deprecated in C++17 and produces an error when building using MSVC as C++20.

srtcore/group.cpp

maxsharabayko · 2022-07-21T10:07:16Z

macOS builds are failing in Travis CI.

error: unknown warning group '-Wambiguous-reversed-operator', ignored [-Werror,-Wunknown-warning-option]
#pragma clang diagnostic ignored "-Wambiguous-reversed-operator"

oviano · 2022-07-21T10:12:58Z

macOS builds are failing in Travis CI.

error: unknown warning group '-Wambiguous-reversed-operator', ignored [-Werror,-Wunknown-warning-option]
#pragma clang diagnostic ignored "-Wambiguous-reversed-operator"

I can see:

osx_image: xcode11.1

I'm building with latest Xcode, 13.4.1. Presumably it uses a newer clang version.

srtcore/group.cpp

ethouris · 2022-07-25T07:32:17Z

srtcore/platform_sys.h

+#if defined(__clang__)
+#pragma clang diagnostic ignored "-Wshorten-64-to-32"
+#pragma clang diagnostic ignored "-Wambiguous-reversed-operator"
+#endif


Could you please paste, somewhere in comments maybe, where exactly these warnings are being reported?

In file included from /Users/oliver/Git/source/ovlibs/ovnet/thirdparty/srt/srtcore/window.cpp:58: In file included from thirdparty/srt/srtcore/window.h:61: thirdparty/srt/srtcore/packet.h:157:54: error: implicit conversion loses integer precision: 'size_t' (aka 'unsigned long') to 'uint32_t' (aka 'unsigned int') [-Werror,-Wshorten-64-to-32] return SEQNO_CONTROL::mask | SEQNO_MSGTYPE::wrap(size_t(type)); ~~~~~~~~~~~~~ ^~~~~~~~~~~~ /Users/oliver/Git/source/ovlibs/ovnet/thirdparty/srt/srtcore/window.cpp:96:29: error: implicit conversion loses integer precision: 'long long' to 'const int' [-Werror,-Wshorten-64-to-32] const int rtt = count_microseconds(currtime - r_aSeq[i].tsTimeStamp); ~~~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /Users/oliver/Git/source/ovlibs/ovnet/thirdparty/srt/srtcore/window.cpp:115:38: error: implicit conversion loses integer precision: 'unsigned long' to 'int' [-Werror,-Wshorten-64-to-32] for (int j = r_iTail, n = r_iHead + size; j < n; ++ j) ~ ~~~~~~~~^~~~~~ /Users/oliver/Git/source/ovlibs/ovnet/thirdparty/srt/srtcore/window.cpp:125:26: error: implicit conversion loses integer precision: 'long long' to 'const int' [-Werror,-Wshorten-64-to-32] const int rtt = count_microseconds(currtime - r_aSeq[j].tsTimeStamp); ~~~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /Users/oliver/Git/source/ovlibs/ovnet/thirdparty/srt/srtcore/window.cpp:179:24: error: implicit conversion loses integer precision: 'size_t' (aka 'unsigned long') to 'int' [-Werror,-Wshorten-64-to-32] for (int i = 0, n = asize; i < n; ++ i) ~ ^~~~~ /Users/oliver/Git/source/ovlibs/ovnet/thirdparty/srt/srtcore/window.cpp:195:17: error: implicit conversion loses integer precision: 'unsigned long' to 'int' [-Werror,-Wshorten-64-to-32] bytesps = (unsigned long)ceil(1000000.0 / (double(sum) / double(bytes))); ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /Users/oliver/Git/source/ovlibs/ovnet/thirdparty/srt/srtcore/window.cpp:243:24: error: implicit conversion loses integer precision: 'size_t' (aka 'unsigned long') to 'int' [-Werror,-Wshorten-64-to-32] for (int i = 0, n = psize; i < n; ++ i) ~ ^~~~~ 7 errors generated.

In file included from /Users/oliver/Git/source/ovlibs/ovnet/thirdparty/srt/srtcore/core.cpp:66: In file included from thirdparty/srt/srtcore/api.h:63: thirdparty/srt/srtcore/cache.h:116:20: error: ISO C++20 considers use of overloaded operator '==' (with operand types 'srt::CInfoBlock' and 'srt::CInfoBlock') to be ambiguous despite there being a unique best viable function [-Werror,-Wambiguous-reversed-operator] if (*data == ***i) ~~~~~ ^ ~~~~ /Users/oliver/Git/source/ovlibs/ovnet/thirdparty/srt/srtcore/core.cpp:4606:19: note: in instantiation of member function 'srt::CCache<srt::CInfoBlock>::lookup' requested here if (m_pCache->lookup(&ib) >= 0) ^ In file included from /Users/oliver/Git/source/ovlibs/ovnet/thirdparty/srt/srtcore/core.cpp:66: In file included from thirdparty/srt/srtcore/api.h:63: thirdparty/srt/srtcore/cache.h:256:9: note: ambiguity is between a regular call to this operator and a call with the argument order reversed bool operator==(const CInfoBlock& obj); ^ thirdparty/srt/srtcore/cache.h:146:20: error: ISO C++20 considers use of overloaded operator '==' (with operand types 'srt::CInfoBlock' and 'srt::CInfoBlock') to be ambiguous despite there being a unique best viable function [-Werror,-Wambiguous-reversed-operator] if (*data == ***i) ~~~~~ ^ ~~~~ /Users/oliver/Git/source/ovlibs/ovnet/thirdparty/srt/srtcore/core.cpp:6221:19: note: in instantiation of member function 'srt::CCache<srt::CInfoBlock>::update' requested here m_pCache->update(&ib); ^ In file included from /Users/oliver/Git/source/ovlibs/ovnet/thirdparty/srt/srtcore/core.cpp:66: In file included from thirdparty/srt/srtcore/api.h:63: thirdparty/srt/srtcore/cache.h:256:9: note: ambiguity is between a regular call to this operator and a call with the argument order reversed bool operator==(const CInfoBlock& obj); ^ thirdparty/srt/srtcore/cache.h:179:28: error: ISO C++20 considers use of overloaded operator '==' (with operand types 'srt::CInfoBlock' and 'srt::CInfoBlock') to be ambiguous despite there being a unique best viable function [-Werror,-Wambiguous-reversed-operator] if (*last_data == ***i) ~~~~~~~~~~ ^ ~~~~ thirdparty/srt/srtcore/cache.h:256:9: note: ambiguity is between a regular call to this operator and a call with the argument order reversed bool operator==(const CInfoBlock& obj); ^ 3 errors generated.

I think breaking the gauge that shows a problem is not the best solution of the problem.

Implicit conversion of type to size_t to be next used as uint32_t should be fixed by converting it first to uint32_t.

The value of rtt if assigned from int64_t should be of type int64_t, not int. Further passing it to any calculations relying on int should then use an explicit conversion.

Likely j in the loop, unless there's a risk of a negative value, should also use the size_t type.

bytesps gets assigned a value that is explicitly converted - should then use the exact type of bytesps.

The overloaded operator== for CInfoBlock likely lacks const.

Would you be able to try these above and retest if this also fixes the problems? If not, would you be able to send the list of the remaining problems so that I can prepare another fix?

Ok.

Maybe better to just cast to int straightaway from count_microseconds(currtime - r_aSeq[j].tsTimeStamp) as the precedent in SRT seems to be that RTT is an int.

If you do this, you get warnings further down, for example r_iTail = (i + 1) % size now complains because r_iTail is an int, but the calculation is now size_t. Basically there is a mismatch throughout SRT where sizes are sometimes size_t, but the rest of the time int. Sometimes you set int to negative numbers/return (-1) to indicate an invalid value, so size_t can't be used everywhere. You could use ssize_t but that's not supported everywhere, so maybe it's just better to cast to (int) to remove these warnings?

and 5. yes, trivial changes. I've done 5 already.

Well, in (3) you can alternatively do r_iHead + int(size).

It's not about silencing warnings, but precisely defining your intentions.

Ok, I've worked through the remaining clang ones and casted in the way that makes sense to me.

Just going to re-enable the Windows warnings I had disabled, and fix any remaining ones on that platform.

It would be good if someone could just check my changes though to make sure.

Just working through the remaining Windows ones.

This code:

// Note: Intentionally the "fixed char size" types are used despite using // character size dependent FormatMessage (instead of FormatMessageA) so that // your compilation fails when you use wide characters. // The problem is that when TCHAR != char, then the buffer written this way // would have to be converted to ASCII, not just copied by strncpy. FormatMessage(0 | FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS, NULL, // no lpSource errnum, // dwMessageId (as controlled by FORMAT_MESSAGE_FROM_SYSTEM) MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), // This below parameter normally should contain a pointer to an allocated buffer, // and this way it's LPTSTR. But when FORMAT_MESSAGE_ALLOCATE_BUFFER, then it is // expected to be a the value of LPTSTR* type, converted to LPTSTR, that designates // a pointer to a variable of type LPTSTR, to which the newly allocated buffer is // assigned. This buffer should be freed afterwards using LocalFree(). (LPSTR)&lpMsgBuf, 0, NULL); if (lpMsgBuf) { strncpy(buf, lpMsgBuf, buflen-1); buf[buflen-1] = 0; LocalFree((HLOCAL)lpMsgBuf); } else { SysStrError_Fallback(errnum, buf, buflen); }

Gives the warning incompatible types - from 'LPSTR' to 'LPWSTR'

Looking at the comment above - it seems to suggest we can't use SRT when compiled with wide characters. Thing is, SRT works just fine in my experience (many years) when compiled as wide char, so what would be the preferred workaround to deal with this warning? Can't we just use FormatMessageA?

Looks like so. Especially that you skipped this line:

const char* lpMsgBuf;

Which should be const TCHAR* lpMsgBuf or something like that, and a similar transformation would have to be applied to the pointer type and likely the strncpy call as well, that is, it would have to change the encoding as well.

What I mean is, SysStrError is expected to return a char* to the caller (to write into log macros, etc) so why does this function need to anything with wide chars at all, it can just use FormatMessageA?

Well, as long as this form is perfectly portable (that is, Windows doesn't recommend to use FormatMessageA directly), that would be a better solution. @jlsantiago0 what do you think?

The clang version with latest Xcode gives a warning about the use of sprintf. The proper fix would be to replace sprintf with snprintf but I'm not too sure of the cross-platform cross-compiler implications of this.

codecov-commenter · 2023-01-15T18:15:47Z

Codecov Report

Merging #2411 (186cc06) into master (a08a42c) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2411      +/-   ##
==========================================
- Coverage   66.20%   66.19%   -0.02%     
==========================================
  Files          99       99              
  Lines       19841    19841              
==========================================
- Hits        13135    13133       -2     
- Misses       6706     6708       +2

Impacted Files	Coverage Δ
haicrypt/cryspr.c	`23.11% <ø> (ø)`
haicrypt/hcrypt_ctx_rx.c	`63.63% <ø> (ø)`
haicrypt/hcrypt_ctx_tx.c	`53.01% <ø> (ø)`
haicrypt/hcrypt_rx.c	`25.86% <ø> (ø)`
srtcore/cache.h	`80.00% <0.00%> (-3.08%)`	⬇️
srtcore/window.cpp	`81.81% <0.00%> (-1.30%)`	⬇️
srtcore/buffer_rcv.cpp	`58.94% <0.00%> (-0.59%)`	⬇️
srtcore/congctl.cpp	`80.82% <0.00%> (-0.52%)`	⬇️
srtcore/list.cpp	`80.45% <0.00%> (-0.26%)`	⬇️
srtcore/core.cpp	`60.19% <0.00%> (ø)`
... and 2 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

oviano · 2023-01-16T14:39:27Z

Ok, I've removed the last warning pragmas and it builds for me on Win32, WinRT, Android, macOS, iOS and tvOS.

My Windows environment is VS2022, so hopefully your CI will pick up any issues with earlier versions of VS, but it seems to have failed above on a Linux test so maybe hasn't got that far.

ethouris · 2023-01-17T09:00:40Z

Note that merging it may take some more time because Max is on vacation this week.

oviano · 2023-01-17T09:01:46Z

Note that merging it may take some more time because Max is on vacation this week.

No problem!

Once merged, would you consider enabling warnings as errors so we can keep a lid on these warnings in future?

ethouris · 2023-01-17T09:03:42Z

I'm not sure if this is a good idea to do this by default. Adding an option for CMake build should do it, I hope.

ethouris · 2023-01-17T17:11:54Z

Fixes #1646

maxsharabayko · 2023-02-09T11:35:14Z

The Travis CI failure is unrelated to these changes. Merging.

maxsharabayko added this to the Next Release milestone Jul 20, 2022

maxsharabayko added Type: Maintenance Work required to maintain or clean up the code [core] Area: Changes in SRT library core labels Jul 20, 2022

maxsharabayko reviewed Jul 20, 2022

View reviewed changes

srtcore/group.cpp Outdated Show resolved Hide resolved

ethouris reviewed Jul 25, 2022

View reviewed changes

srtcore/group.cpp Outdated Show resolved Hide resolved

ethouris reviewed Jul 25, 2022

View reviewed changes

maxsharabayko modified the milestones: v1.5.1, Next release Sep 12, 2022

oviano force-pushed the oviano-mods branch from 4f74f76 to 7a663b3 Compare September 13, 2022 09:14

oviano added 4 commits January 15, 2023 16:29

Disabled some warnings

025574c

Include platform_sys.h in some files to disable warnings

2a3bf74

DIsabled deprecated declarations warning under clang

9ad1f50

The clang version with latest Xcode gives a warning about the use of sprintf. The proper fix would be to replace sprintf with snprintf but I'm not too sure of the cross-platform cross-compiler implications of this.

Include platform_sys.h in cryspr.c

186cc06

oviano force-pushed the oviano-mods branch from e55f0dd to 186cc06 Compare January 15, 2023 17:05

oviano added 6 commits January 16, 2023 07:44

Proper fix for ambiguous-reversed-operator warning

282a1ad

Proper fix for deprecated-declarations warning

86c3c81

Proper fixes for shorten-64-to-32 (clang)

1796434

Proper fixes for MSVC warnings 4101 4133 4244 4267

c4506ad

Remove disabling of MSVC warnings 4251 26712

2635e09

Removed last remaining disable warning pragma

8743fb6

ethouris approved these changes Jan 17, 2023

View reviewed changes

Merge branch 'master' into oviano-mods

2250ca7

maxsharabayko merged commit 21b55a2 into Haivision:master Feb 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disabled warnings various platforms and fixed C++20 Windows build #2411

Disabled warnings various platforms and fixed C++20 Windows build #2411

oviano commented Jul 20, 2022

maxsharabayko commented Jul 21, 2022

oviano commented Jul 21, 2022

ethouris Jul 25, 2022

oviano Aug 6, 2022

ethouris Nov 10, 2022

oviano Jan 16, 2023 •

edited

Loading

ethouris Jan 16, 2023

oviano Jan 16, 2023 •

edited

Loading

oviano Jan 16, 2023 •

edited

Loading

ethouris Jan 16, 2023 •

edited

Loading

oviano Jan 16, 2023

ethouris Jan 16, 2023

codecov-commenter commented Jan 15, 2023

oviano commented Jan 16, 2023

ethouris commented Jan 17, 2023

oviano commented Jan 17, 2023

ethouris commented Jan 17, 2023

ethouris commented Jan 17, 2023

maxsharabayko commented Feb 9, 2023

Disabled warnings various platforms and fixed C++20 Windows build #2411

Disabled warnings various platforms and fixed C++20 Windows build #2411

Conversation

oviano commented Jul 20, 2022

maxsharabayko commented Jul 21, 2022

oviano commented Jul 21, 2022

ethouris Jul 25, 2022

Choose a reason for hiding this comment

oviano Aug 6, 2022

Choose a reason for hiding this comment

ethouris Nov 10, 2022

Choose a reason for hiding this comment

oviano Jan 16, 2023 • edited Loading

Choose a reason for hiding this comment

ethouris Jan 16, 2023

Choose a reason for hiding this comment

oviano Jan 16, 2023 • edited Loading

Choose a reason for hiding this comment

oviano Jan 16, 2023 • edited Loading

Choose a reason for hiding this comment

ethouris Jan 16, 2023 • edited Loading

Choose a reason for hiding this comment

oviano Jan 16, 2023

Choose a reason for hiding this comment

ethouris Jan 16, 2023

Choose a reason for hiding this comment

codecov-commenter commented Jan 15, 2023

Codecov Report

oviano commented Jan 16, 2023

ethouris commented Jan 17, 2023

oviano commented Jan 17, 2023

ethouris commented Jan 17, 2023

ethouris commented Jan 17, 2023

maxsharabayko commented Feb 9, 2023

oviano Jan 16, 2023 •

edited

Loading

oviano Jan 16, 2023 •

edited

Loading

oviano Jan 16, 2023 •

edited

Loading

ethouris Jan 16, 2023 •

edited

Loading