chfast / intx

Extended precision integer C++ library
Apache License 2.0
122 stars 34 forks source link

Use implementation independent attributes #284

Closed 0xAltCunningham closed 1 year ago

0xAltCunningham commented 1 year ago

Fixes #285.

codecov-commenter commented 1 year ago

Codecov Report

Merging #284 (aecdc2f) into master (f3b775f) will not change coverage. The diff coverage is n/a.

:exclamation: Current head aecdc2f differs from pull request most recent head f5f0bba. Consider uploading reports for the commit f5f0bba to get more accurate results

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff            @@
##            master      #284   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines         1951      1951           
=========================================
  Hits          1951      1951           
Flag Coverage Δ
32bit 100.00% <ø> (ø)
gcc 99.48% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
include/intx/intx.hpp 100.00% <ø> (ø)
axic commented 1 year ago

We run four MSVC compiler versions (see recent builds here and they seem to succeed.

What compiler version do you see this problem with? Can you point us to some logs / repository?

0xAltCunningham commented 1 year ago

I'm seeing this problem with MSVC 19.34.31942.0 (MSBuild v17.4.1+9a89d02ff). It's the current version used in GitHub Actions when the windows-latest image is selected. Here's an example of the warning being thrown in a build run: https://github.com/0xAltCunningham/evmtools/actions/runs/4076238955/jobs/7271225492#step:5:19

0xAltCunningham commented 1 year ago

I got the compiler version wrong on my previous comment, that was just the MSBuild version.

When rerunning the job I realized the compiler version is actually MSVC 19.34.31942.0:

-- Building for: Visual Studio 17 2022
-- The CXX compiler identification is MSVC 19.34.31942.0
0xAltCunningham commented 1 year ago

Not sure if related, but something else that I think could be the cause of this warning is that I'm using CXX_STANDARD 23 when building my project.

axic commented 1 year ago

Interesting, we do have an MSVC 2022 build. Will need to wait for @chfast, maybe he has some insight.

axic commented 1 year ago

@chfast ping

chfast commented 1 year ago

We specifically disable this warning because the idea of C++ attributes is to wrap compiler-specific features (so it is expected some attributes are going to be unknown to some compilers). Otherwise, you end up with #if hell. https://github.com/chfast/intx/blob/master/cmake/CableCompilerSettings.cmake#L135-L136

chfast commented 1 year ago

This has been fixed by #286. Let me know if this works for you.

0xAltCunningham commented 1 year ago

Hey @chfast, the changes introduced in #286 work for me (Windows CI run)! Thanks for taking the time to fix it, and I understand now that adding #ifs to the functions would be a messy solution.

Thanks @axic as well, for the prompt responses and attention paid to this issue.

chfast commented 1 year ago

I understand now that adding #ifs to the functions would be a messy solution

I think for intx.hpp this is acceptable. After the change the file should build without any warnings disabled. For intx/test I decided to disable unknown pragma warnings but this should affect users much less.

chfast commented 1 year ago

I will make intx release soon.