windows: Fix remaining compiler warnings (MSVC) #14151

practicalswift commented at 1:09 PM on September 5, 2018: contributor

Fix remaining compiler warnings (MSVC).

Before:

$ msbuild /p:TrackFileAccess=false /p:CLToolExe=clcache.exe build_msvc\bitcoin.sln /m /v:q /nowarn:C4244;C4267;C4715 /nologo
…\script\script.cpp(272): warning C4018: '>': signed/unsigned mismatch
…\test\allocator_tests.cpp(147): warning C4312: 'reinterpret_cast': conversion from 'int' to 'void *' of greater size
…\boost\test\tools\old\impl.hpp(107): warning C4805: '==': unsafe mix of type 'const Left' and type 'const Right' in operation
…\test\crypto_tests.cpp(535): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)
…\test\script_tests.cpp(188): warning C4805: '==': unsafe mix of type 'int' and type 'bool' in operation
…\test\script_tests.cpp(190): warning C4805: '==': unsafe mix of type 'int' and type 'bool' in operation
…\test\script_tests.cpp(191): warning C4805: '==': unsafe mix of type 'int' and type 'bool' in operation
$

After:

$ msbuild /p:TrackFileAccess=false /p:CLToolExe=clcache.exe build_msvc\bitcoin.sln /m /v:q /nowarn:C4244;C4267;C4715;C4805 /nologo
$

fanquake added the label Windows on Sep 5, 2018

practicalswift renamed this:
~~Fix remaining compiler warnings (MSVC)~~
windows: Fix remaining compiler warnings (MSVC)
on Sep 5, 2018

in src/script/script.cpp:272 in 633c6d997f outdated

 268 | @@ -269,7 +269,7 @@ bool CScript::HasValidOps() const
 269 |      while (it < end()) {
 270 |          opcodetype opcode;
 271 |          std::vector<unsigned char> item;
 272 | -        if (!GetOp(it, opcode, item) || opcode > MAX_OPCODE || item.size() > MAX_SCRIPT_ELEMENT_SIZE) {
 273 | +        if (!GetOp(it, opcode, item) || opcode > MAX_OPCODE || item.size() > (size_t)MAX_SCRIPT_ELEMENT_SIZE) {

MarcoFalke commented at 1:15 PM on September 5, 2018:

MAX_SCRIPT_ELEMENT_SIZE is of type unsigned, so I don't see why this change is here.

practicalswift commented at 1:50 PM on September 5, 2018:

Sorry, should have been (unsigned int)opcode. Now updated. Please re-review :-)

practicalswift force-pushed on Sep 5, 2018

in src/script/script.cpp:272 in 699d458053 outdated

 268 | @@ -269,7 +269,7 @@ bool CScript::HasValidOps() const
 269 |      while (it < end()) {
 270 |          opcodetype opcode;
 271 |          std::vector<unsigned char> item;
 272 | -        if (!GetOp(it, opcode, item) || opcode > MAX_OPCODE || item.size() > MAX_SCRIPT_ELEMENT_SIZE) {
 273 | +        if (!GetOp(it, opcode, item) || (unsigned int)opcode > MAX_OPCODE || item.size() > MAX_SCRIPT_ELEMENT_SIZE) {

scravy commented at 12:35 PM on September 6, 2018:

Nit: could be a static_cast

in src/test/crypto_tests.cpp:535 in 699d458053 outdated

 531 | @@ -532,7 +532,7 @@ BOOST_AUTO_TEST_CASE(countbits_tests)
 532 |              // Check handling of zero.
 533 |              BOOST_CHECK_EQUAL(CountBits(0), 0U);
 534 |          } else if (i < 10) {
 535 | -            for (uint64_t j = 1 << (i - 1); (j >> i) == 0; ++j) {
 536 | +            for (uint64_t j = ((uint64_t) 1) << (i - 1); (j >> i) == 0; ++j) {

scravy commented at 12:35 PM on September 6, 2018:

Nit: could be a static_cast

practicalswift force-pushed on Sep 6, 2018

sipsorcery commented at 8:26 PM on September 8, 2018: member

tACK https://github.com/bitcoin/bitcoin/pull/14151/commits/e99684c84094d5f04382d91eed00ec3ef911fd03

Prior to this PR:

msbuild /p:TrackFileAccess=false /p:CLToolExe=clcache.exe build_msvc\bitcoin.sln /m /v:q /nowarn:C4244;C4267;C4715 /nologo
c:\projects\bitcoin-72c17\src\script\script.cpp(272): warning C4018: '>': signed/unsigned mismatch [C:\projects\bitcoin-72c17\build_msvc\libbitcoinconsensus\libbitcoinconsensus.vcxproj]
c:\projects\bitcoin-72c17\src\test\allocator_tests.cpp(147): warning C4312: 'reinterpret_cast': conversion from 'int' to 'void *' of greater size [C:\projects\bitcoin-72c17\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
c:\tools\vcpkg\installed\x64-windows-static\include\boost\test\tools\old\impl.hpp(107): warning C4805: '==': unsafe mix of type 'const Left' and type 'const Right' in operation [C:\projects\bitcoin-72c17\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
c:\projects\bitcoin-72c17\src\test\crypto_tests.cpp(535): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?) [C:\projects\bitcoin-72c17\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
c:\projects\bitcoin-72c17\src\test\script_tests.cpp(188): warning C4805: '==': unsafe mix of type 'int' and type 'bool' in operation [C:\projects\bitcoin-72c17\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
c:\projects\bitcoin-72c17\src\test\script_tests.cpp(190): warning C4805: '==': unsafe mix of type 'int' and type 'bool' in operation [C:\projects\bitcoin-72c17\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
c:\projects\bitcoin-72c17\src\test\script_tests.cpp(191): warning C4805: '==': unsafe mix of type 'int' and type 'bool' in operation [C:\projects\bitcoin-72c17\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
move build_msvc\%PLATFORM%\%CONFIGURATION%\*.iobj build_msvc\cache\

Subsequently:

msbuild /p:TrackFileAccess=false /p:CLToolExe=clcache.exe build_msvc\bitcoin.sln /m /v:q /nowarn:C4244;C4267;C4715;C4805 /nologo
move build_msvc\%PLATFORM%\%CONFIGURATION%\*.iobj build_msvc\cache\

in src/test/allocator_tests.cpp:150 in e99684c840 outdated

 143 | @@ -144,6 +144,10 @@ class TestLockedPageAllocator: public LockedPageAllocator
 144 |                  *lockingSuccess = true;
 145 |              }
 146 |  
 147 | +#ifdef _MSC_VER
 148 | +// Disable MSVC warning: C4312: 'reinterpret_cast': conversion from 'int' to 'void *' of greater size
 149 | +#pragma warning(disable: 4312)
 150 | +#endif

MarcoFalke commented at 6:14 PM on September 9, 2018:

Can't this be solved by return reinterpret_cast<void*>(uint64_t{0x08000000 + (count<<24)});?

practicalswift force-pushed on Sep 9, 2018

practicalswift commented at 7:51 PM on September 9, 2018: contributor

@sipsorcery @MarcoFalke Please re-review :-)

practicalswift commented at 5:27 AM on September 10, 2018: contributor

@ken2812221 Squashed. Please re-review :-)

practicalswift force-pushed on Sep 10, 2018

in appveyor.yml:38 in 224cc41bb9 outdated

  34 | @@ -35,7 +35,7 @@ before_build:
  35 |         }
  36 |  - ps:  Start-Process clcache-server
  37 |  build_script:
  38 | -- cmd: msbuild /p:TrackFileAccess=false /p:CLToolExe=clcache.exe build_msvc\bitcoin.sln /m /v:q /nowarn:C4244;C4267;C4715 /nologo
  39 | +- cmd: msbuild /p:TrackFileAccess=false /p:CLToolExe=clcache.exe build_msvc\bitcoin.sln /m /v:q /nowarn:C4244;C4267;C4715;C4805 /nologo

NicolasDorier commented at 5:46 AM on September 10, 2018:

I think instead of bundling that in /nowarn we should include the nowarn inside the generated project file.

NicolasDorier commented at 5:46 AM on September 10, 2018:

<PropertyGroup><NoWarn>C4244;C4267;C4715;C4805</NoWarn></PropertyGroup>

practicalswift commented at 11:07 AM on September 10, 2018:

@NicolasDorier I'll fix that! I'm not really familiar with the MSVC project files - in which of them should that be included?

These are the files with PropertyGroup:

$ git grep -l '<PropertyGroup'
build_msvc/bench_bitcoin/bench_bitcoin.vcxproj
build_msvc/bitcoin-cli/bitcoin-cli.vcxproj
build_msvc/bitcoin-tx/bitcoin-tx.vcxproj
build_msvc/bitcoind/bitcoind.vcxproj
build_msvc/common.init.vcxproj
build_msvc/common.vcxproj
build_msvc/libbitcoin_cli/libbitcoin_cli.vcxproj.in
build_msvc/libbitcoin_common/libbitcoin_common.vcxproj.in
build_msvc/libbitcoin_crypto/libbitcoin_crypto.vcxproj.in
build_msvc/libbitcoin_qt/libbitcoin_qt.vcxproj
build_msvc/libbitcoin_server/libbitcoin_server.vcxproj.in
build_msvc/libbitcoin_util/libbitcoin_util.vcxproj.in
build_msvc/libbitcoin_wallet/libbitcoin_wallet.vcxproj.in
build_msvc/libbitcoin_zmq/libbitcoin_zmq.vcxproj.in
build_msvc/libbitcoinconsensus/libbitcoinconsensus.vcxproj
build_msvc/libunivalue/libunivalue.vcxproj
build_msvc/test_bitcoin-qt/test_bitcoin-qt.vcxproj
build_msvc/test_bitcoin/test_bitcoin.vcxproj
build_msvc/testconsensus/testconsensus.vcxproj

practicalswift commented at 12:51 PM on September 10, 2018:

Tried this but it didn't suppress the warnings:

diff --git a/appveyor.yml b/appveyor.yml
index 147f458d9..ae6bd2f89 100644
--- a/appveyor.yml
+++ b/appveyor.yml
@@ -35,7 +35,7 @@ before_build:
        }
 - ps:  Start-Process clcache-server
 build_script:
-- cmd: msbuild /p:TrackFileAccess=false /p:CLToolExe=clcache.exe build_msvc\bitcoin.sln /m /v:q /nowarn:C4244;C4267;C4715;C4805 /nologo
+- cmd: msbuild /p:TrackFileAccess=false /p:CLToolExe=clcache.exe build_msvc\bitcoin.sln /m /v:q /nologo
 after_build:
 - cmd: move build_msvc\%PLATFORM%\%CONFIGURATION%\*.iobj build_msvc\cache\
 - cmd: move build_msvc\%PLATFORM%\%CONFIGURATION%\*.ipdb build_msvc\cache\
diff --git a/build_msvc/common.init.vcxproj b/build_msvc/common.init.vcxproj
index c3c0f665c..4ba2aca6c 100644
--- a/build_msvc/common.vcxproj
+++ b/build_msvc/common.vcxproj
@@ -7,9 +7,12 @@
           $(BuildDependsOn);
       </BuildDependsOn>
   </PropertyGroup>
+  <PropertyGroup>
+      <NoWarn>C4244;C4267;C4715;C4805</NoWarn>
+  </PropertyGroup>
   <Target Name="CopyConfig"
           Inputs="$(MSBuildThisFileDirectory)bitcoin_config.h"
           Outputs="$(MSBuildThisFileDirectory)..\src\config\bitcoin-config.h">
       <Copy SourceFiles="$(MSBuildThisFileDirectory)bitcoin_config.h" DestinationFiles="$(MSBuildThisFileDirectory)..\src\config\bitcoin-config.h" />
   </Target>

NicolasDorier commented at 12:55 PM on September 10, 2018:

You normally did right. I take a look.

NicolasDorier commented at 1:22 PM on September 10, 2018:

Nevermind this is only a C# thing :/ We can level down the WarningLevel in the project file, but can't control individual warning... :/

practicalswift force-pushed on Sep 10, 2018

NicolasDorier commented at 1:38 PM on September 10, 2018: contributor

Adding /warnaserror is a bit too much?

NicolasDorier commented at 1:48 PM on September 10, 2018: contributor

For test, I tried running a dry compile with /warnaserror, it worked fine but two warning show up. (If I recompile a second time, without cleaning, it does not show)

"C:\Sources\bitcoin\build_msvc\bitcoin.sln" (default target) (1) ->
"C:\Sources\bitcoin\build_msvc\libbitcoinconsensus\libbitcoinconsensus.vcxproj" (default target) (2) ->
(ClCompile target) ->
  c:\program files (x86)\microsoft visual studio\2017\buildtools\vc\tools\msvc\14.14.26428\include\xutility(3182): error C4996: 'std::equal::_Unchecked_iterators::_Deprecate': Call to 'st
d::equal' with parameters that may be unsafe - this call relies on the caller to check that the passed values are correct. To disable this warning, use -D_SCL_SECURE_NO_WARNINGS. See docu
mentation on how to use Visual C++ 'Checked Iterators' [C:\Sources\bitcoin\build_msvc\libbitcoinconsensus\libbitcoinconsensus.vcxproj]


"C:\Sources\bitcoin\build_msvc\bitcoin.sln" (default target) (1) ->
"C:\Sources\bitcoin\build_msvc\testconsensus\testconsensus.vcxproj" (default target) (3) ->
"C:\Sources\bitcoin\build_msvc\libbitcoin_util\libbitcoin_util.vcxproj" (default target) (4) ->
(Lib target) ->
  sync.obj : error LNK4221: This object file does not define any previously undefined public symbols, so it will not be used by any link operation that consumes this library [C:\Sources\b
itcoin\build_msvc\libbitcoin_util\libbitcoin_util.vcxproj]

That said, I don't think this is very important,

tACK 224cc41bb9fd2906d9a61abd0cad9bd77365569b

practicalswift commented at 1:55 PM on September 10, 2018: contributor

@NicolasDorier Personally I'm all for /warnaserror assuming AppVeyor build failures don't block merge (and thus get in the way of the maintainers job). They don't right?

NicolasDorier commented at 2:38 PM on September 10, 2018: contributor

The whole purpose of /warnaserror is to break the build. I tried /warnaserror, and there is the above error for the first build. I don't think it is worth fixing/fixable.

practicalswift commented at 3:02 PM on September 10, 2018: contributor

@NicolasDorier Yes, I know but I thought the project was configured to block merge in case of Travis build failure but not in case of AppVeyor build failure. But I'm probably mistaken :-)

NicolasDorier commented at 3:22 PM on September 10, 2018: contributor

ah yes I think you are right. Well, I think we can ignore /warnaserror for now, except if you find an obvious fix, even if it does not break, I feel uneasy with red failure. :p

sipsorcery commented at 8:59 PM on September 11, 2018: member

For a C++ build to exclude specific warnings and set the "treat warnings as error" flag the following node can be added to the common.vcxproj file:

<Project>
  <ItemDefinitionGroup>
	<ClCompile>
	  <DisableSpecificWarnings>4244;4267;4715;4805;</DisableSpecificWarnings>
	  <TreatWarningAsError>true</TreatWarningAsError>
	</ClCompile>
  </ItemDefinitionGroup>
</Project>

When warnings as error is set the script\interpreter.cpp code fails to compile on line 264 and specifically this check:

std::equal(b.begin(), b.end(), pc)

A quick Google found a blog post about Visual C++ range checking safety. To ignore this warning add 4996 to the list of specific warnings and the solution will then compile.

sipsorcery commented at 7:01 PM on September 12, 2018: member

tACK https://github.com/bitcoin/bitcoin/pull/14151/commits/f14f4a68850947f04ab6f94ec6e52a07fb1cd61f.

MarcoFalke added the label Validation on Sep 14, 2018

MarcoFalke removed the label Windows on Sep 14, 2018

MarcoFalke added the label Windows on Sep 14, 2018

DrahtBot commented at 11:24 PM on September 14, 2018: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15045 ([test] Apply maximal flags to tx_valid tests and minimal flags to tx_invalid tests by jl2012)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Needs rebase on Sep 25, 2018

practicalswift force-pushed on Sep 25, 2018

practicalswift commented at 9:34 PM on September 25, 2018: contributor

Rebased!

DrahtBot removed the label Needs rebase on Sep 25, 2018

NicolasDorier commented at 4:23 AM on September 26, 2018: contributor

tACK

practicalswift commented at 7:59 AM on October 1, 2018: contributor

@sipsorcery @ken2812221 @MarcoFalke Thanks for reviewing. Would you mind re-reviewing? :-)

in src/test/script_tests.cpp:187 in 26a9c9e4ab outdated

 183 | @@ -184,11 +184,12 @@ void DoTest(const CScript& scriptPubKey, const CScript& scriptSig, const CScript
 184 |      stream << tx2;
 185 |      int libconsensus_flags = flags & bitcoinconsensus_SCRIPT_FLAGS_VERIFY_ALL;
 186 |      if (libconsensus_flags == flags) {
 187 | +        int expectedSuccessCode = expect ? 1 : 0;

sipsorcery commented at 8:26 AM on October 1, 2018:

Bug? Should it be int expectedSuccessCode == expect ? 1 : 0;?

practicalswift commented at 8:33 AM on October 1, 2018:

No, we want expectedSuccessCode to be 1 (if expect) or 0 (if !expect) :-)

ken2812221 commented at 8:34 AM on October 1, 2018:

This is correct, the integer value is assigned to expectedSuccessCode.

sipsorcery commented at 11:24 AM on October 1, 2018: member

tACK https://github.com/bitcoin/bitcoin/pull/14151/commits/26a9c9e4ab225cdb276ff8bfde4b09fd939a3bae.

NicolasDorier commented at 1:39 PM on October 2, 2018: contributor

ping @MarcoFalke, anything missing for merge?

MarcoFalke commented at 5:04 AM on October 3, 2018: member

Generally we don't touch consensus code to silence compiler warnings of some specific compiler version, since there is (to the best of my knowledge) no method yet to review the changes for correctness. Reviewers well-versed with compiler internals and language specification could look for issues, but a formal method to prove correctness is currently not existing.

practicalswift force-pushed on Oct 3, 2018

practicalswift commented at 7:41 AM on October 3, 2018: contributor

@MarcoFalke Now suppressing the warning in src/script/script.cpp instead. Would you mind re-reviewing? :-)

practicalswift force-pushed on Oct 3, 2018

DrahtBot added the label Needs rebase on Oct 20, 2018

practicalswift force-pushed on Oct 20, 2018

practicalswift commented at 11:54 PM on October 20, 2018: contributor

Rebased! :-)

DrahtBot removed the label Needs rebase on Oct 21, 2018

NicolasDorier commented at 10:44 AM on October 21, 2018: contributor

reACK, can it be merged? This has been ACK several time already.

practicalswift commented at 8:47 AM on November 7, 2018: contributor

Can I do anything to increase the likelihood of getting this PR merge? :-)

NicolasDorier commented at 9:59 AM on November 7, 2018: contributor

Dance on one foot clapping your hands singing praise to the genesis block.

ken2812221 commented at 7:07 AM on November 9, 2018: contributor

ACK 896eddb28aae304d8f380daba5abb38fe134e49e

Empact commented at 7:57 PM on November 12, 2018: member

nit: somewhat prefer ull postfix over static_cast<uint64_t>(literal)

utACK 896eddb

practicalswift force-pushed on Nov 13, 2018

practicalswift commented at 3:07 PM on January 5, 2019: contributor

@MarcoFalke Would you mind re-reviewing? Let me know if I can change anything in order to get your ACK/utACK :-)

in src/script/script.cpp:288 in 896eddb28a outdated

 281 | @@ -276,6 +282,11 @@ bool CScript::HasValidOps() const
 282 |      return true;
 283 |  }
 284 |  
 285 | +#ifdef _MSC_VER
 286 | +// Re-enable MSVC warnings after C4018 suppression in CScript::HasValidOps()
 287 | +#pragma warning(pop)
 288 | +#endif

MarcoFalke commented at 9:17 AM on January 6, 2019:

NACK on adding these. It makes the code harder to parse as a human and they don't add any value.

practicalswift force-pushed on Jan 6, 2019

practicalswift commented at 10:06 AM on January 6, 2019: contributor

@MarcoFalke Thanks for reviewing. Feedback addressed. Please re-review :-)

Now only touching source files in src/test/.

practicalswift commented at 2:26 PM on January 7, 2019: contributor

@sipsorcery @NicolasDorier @ken2812221 Would you mind re-utACK after re-reviewing? :-)

NicolasDorier commented at 2:32 PM on January 7, 2019: contributor

re-utACK for me

sipsorcery commented at 7:20 AM on January 8, 2019: member

tACK https://github.com/bitcoin/bitcoin/commit/bc7aff79fc0cfb10d25a7dd5928a1cb8c36a063b & https://github.com/bitcoin/bitcoin/pull/14151/commits/1423334fa0b45159c2304fda9e768d156e89f4a4.

ken2812221 commented at 5:20 PM on January 13, 2019: contributor

ACK bc7aff79fc0cfb10d25a7dd5928a1cb8c36a063b

Empact commented at 6:11 PM on January 13, 2019: member

re-utACK https://github.com/bitcoin/bitcoin/pull/14151/commits/bc7aff79fc0cfb10d25a7dd5928a1cb8c36a063b

in src/test/crypto_tests.cpp:535 in bc7aff79fc outdated

 531 | @@ -532,7 +532,7 @@ BOOST_AUTO_TEST_CASE(countbits_tests)
 532 |              // Check handling of zero.
 533 |              BOOST_CHECK_EQUAL(CountBits(0), 0U);
 534 |          } else if (i < 10) {
 535 | -            for (uint64_t j = 1 << (i - 1); (j >> i) == 0; ++j) {
 536 | +            for (uint64_t j = static_cast<uint64_t>(1) << (i - 1); (j >> i) == 0; ++j) {

laanwj commented at 3:29 PM on January 15, 2019:

eeek; is this really the only way to write this?

practicalswift commented at 3:34 PM on January 15, 2019:

@laanwj static_cast<uint64_t>(1) is in line with what the C++ Core Guidelines recommends: ES.49: If you must use a cast, use a named cast, but (uint64_t)1 is obviously an alternative (so is UINT64_C(1), etc.). Really worth breaking four utACK:s for changing that? :-)

laanwj commented at 3:43 PM on January 15, 2019:

@luke-jr suggested uint64_t(1), that seems more C++ish without being verbose, and seems to work locally (I don't know if it avoids the warning)

sipa commented at 3:47 PM on January 15, 2019:

I generally prefer C-style casts for primitive types (due to syntactic overload).

laanwj commented at 3:51 PM on January 15, 2019:

Really worth breaking four utACK:s for changing that? :-)

Maybe not, though I'd dread to see another PR which—for whatever reason—changes the syntax again later, so I'd rather have agreement on this on first go.

luke-jr commented at 3:58 PM on January 15, 2019:

j is at most 256, so this should work:

for (unsigned j = 1U << (i - 1); (j >> i) == 0; ++j) {

practicalswift commented at 3:57 PM on January 15, 2019: contributor

@laanwj Added a commit. Please re-review :-)

laanwj commented at 4:27 PM on January 15, 2019: member

LGTM, please squash before merge though! (and please, don't tag me in commit messages, this gets me lots of mails)

practicalswift force-pushed on Jan 15, 2019

practicalswift commented at 7:14 PM on January 15, 2019: contributor

@laanwj Fixed! Sorry for the tag. Please re-review :-) @NicolasDorier @sipsorcery @ken2812221 @Empact Would you mind re-reviewing?

Fix remaining compiler warnings (MSVC). Move disabling of specific warnings from /nowarn to project file. b9dafe7d9f

practicalswift force-pushed on Jan 15, 2019

sipsorcery commented at 7:39 PM on January 15, 2019: member

re-utACK b9dafe7d9ffcbe7928ffbfba816b54e196c57664.

Personally I would use an integer-suffix instead of the static cast, e.g. 0x0x08000000ul but it's purely a style thing and of no consequence here.

practicalswift commented at 7:46 PM on January 15, 2019: contributor

@sipsorcery I don't think there exists an integer-suffix in C++11 guaranteeing a width of exactly 64 bits on all supported platforms? :-)

sipsorcery commented at 9:18 PM on January 15, 2019: member

I don't think there exists an integer-suffix in C++11 guaranteeing a width of exactly 64 bits on all supported platforms? :-)

The C++17 specification states that ul can be represented by unsigned long or unsigned long long so no there is no guarantee of exactly 64 bits. It also indicates that uint64_t, uint32_t etc. are optional.

I've no argument either way.

laanwj commented at 12:36 PM on January 16, 2019: member

Personally I would use an integer-suffix instead of the static cast, e.g. 0x0x08000000ul but it's purely a style thing and of no consequence here.

@sipsorcery I don't think there exists an integer-suffix in C++11 guaranteeing a width of exactly 64 bits on all supported platforms? :-)

Yeah, indeed, if only C++ had Rust's u64 suffix it would be easy!

laanwj commented at 12:37 PM on January 16, 2019: member

utACK b9dafe7d9ffcbe7928ffbfba816b54e196c57664

laanwj merged this on Jan 16, 2019

laanwj closed this on Jan 16, 2019

laanwj referenced this in commit 19c60ca497 on Jan 16, 2019

MarcoFalke referenced this in commit 7027c67cac on Jul 2, 2020

practicalswift deleted the branch on Apr 10, 2021

linuxsh2 referenced this in commit 9585edc005 on Sep 16, 2021

Munkybooty referenced this in commit a5ec8ddbcf on Oct 3, 2021

vijaydasmp referenced this in commit dfa262c93f on Oct 4, 2021

Munkybooty referenced this in commit f8c69049c2 on Oct 5, 2021

Munkybooty referenced this in commit 6ef164513c on Oct 7, 2021

pravblockc referenced this in commit a04bd6c8c8 on Nov 18, 2021

DrahtBot locked this on Aug 18, 2022