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

Use implementation independent attributes #284

Conversation

0xAltCunningham
Copy link

@0xAltCunningham 0xAltCunningham commented Feb 16, 2023

Fixes #285.

@0xAltCunningham 0xAltCunningham force-pushed the refactor/implementation-independent-attributes branch from 0beb2bf to aecdc2f Compare February 16, 2023 13:59
@codecov-commenter
Copy link

codecov-commenter commented Feb 16, 2023

Codecov Report

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

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

📣 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
Copy link
Collaborator

axic commented Feb 16, 2023

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
Copy link
Author

0xAltCunningham commented Feb 16, 2023

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
Copy link
Author

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
Copy link
Author

0xAltCunningham commented Feb 17, 2023

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
Copy link
Collaborator

axic commented Feb 17, 2023

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

@axic
Copy link
Collaborator

axic commented Feb 24, 2023

@chfast ping

@chfast
Copy link
Owner

chfast commented Feb 24, 2023

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
Copy link
Owner

chfast commented Feb 25, 2023

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

@chfast chfast closed this Feb 25, 2023
@0xAltCunningham
Copy link
Author

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.

@0xAltCunningham 0xAltCunningham deleted the refactor/implementation-independent-attributes branch February 26, 2023 11:15
@chfast
Copy link
Owner

chfast commented Feb 26, 2023

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
Copy link
Owner

chfast commented Feb 26, 2023

I will make intx release soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Library fails to build with MSVC because of unrecognized attributes
4 participants