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.

Sign up for GitHub

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

Jump to bottom

Support Mantic compilers #3285

Merged
merged 4 commits into from
Nov 21, 2023
Merged

Support Mantic compilers #3285

merged 4 commits into from
Nov 21, 2023

Conversation

luis4a0
Copy link
Contributor

@luis4a0 luis4a0 commented Nov 6, 2023

This fixes compilation with gcc-13 and clang-16.

@luis4a0
Copy link
Contributor Author

luis4a0 commented Nov 6, 2023

Still having issues with the fmt lib with gcc-13.

@luis4a0
Copy link
Contributor Author

luis4a0 commented Nov 6, 2023

Done.

@codecov Codecov
Copy link

codecov bot commented Nov 6, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8098db7) 84.05% compared to head (5d2c179) 84.02%.
Report is 14 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3285      +/-   ##
==========================================
- Coverage   84.05%   84.02%   -0.04%     
==========================================
  Files         250      250              
  Lines       13479    13487       +8     
==========================================
+ Hits        11330    11332       +2     
- Misses       2149     2155       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@luis4a0
Copy link
Contributor Author

luis4a0 commented Nov 6, 2023

Tested it also works with clang-17.

townsend2010
townsend2010 suggested changes Nov 15, 2023
View reviewed changes
Copy link
Contributor

@townsend2010 townsend2010 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine overall, but I have one inline comment and a general question. Why the need to add those escaped quotes in some of the strings?

@luis4a0
Copy link
Contributor Author

luis4a0 commented Nov 15, 2023

Hey @townsend2010, thanks! I added those escape chars because, for some change in fmt, some strings which were outputted with double quotes don't have double quotes now. I wanted the output to be the same as before the change in fmt.

@luis4a0
[ssl cert] Fix signature to avoid compiler error.
8068448
@luis4a0
[ip_address] Make the compiler recognize uint*_t
e62eee0
@luis4a0
[3rd party] Update fmt to 10.1.1.
1f9987c
@luis4a0
Force quoting filesystem::path.
5d2c179 
The version of fmtlib we are using now introduced a breaking change
in the output of filesystem::path, making the default to not quote
the path when formatting. For this reason, and with the aim of
maintaining our output as it was before bumping the version of
fmtlib, we force quoting the paths, using {:?} instead of {}.
@luis4a0
Copy link
Contributor Author

luis4a0 commented Nov 16, 2023

Hey @townsend2010! The default formatting of std::filesystem::path changed in the last version of fmtlib. It used to quote the paths, and now it defaults to not quoting them. Thus I forced quoting where needed, so the tests are not affected by the change.

townsend2010
townsend2010 approved these changes Nov 21, 2023
View reviewed changes
Copy link
Contributor

@townsend2010 townsend2010 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, LGTM, thanks!

@townsend2010 townsend2010 merged commit 4e721d0 into main Nov 21, 2023
@townsend2010 townsend2010 deleted the gcc-13 branch November 21, 2023 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@townsend2010 townsend2010 townsend2010 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@luis4a0 @townsend2010