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

Added cstd detection, and tidied variable names #654

Closed
wants to merge 3 commits into from

Conversation

jhol
Copy link
Contributor

@jhol jhol commented Jul 18, 2024

No description provided.

jhol added 3 commits July 18, 2024 11:10
The cstd setting was added to Conan in v2.4.0. Similar to cppstd, it is
used to indicate the selected C language standard.
@jhol
Copy link
Contributor Author

jhol commented Jul 18, 2024

I don't think the test failures are my fault. Looks like there is something up with the build machine.

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @jhol

I think the main contribution is fine, I need to double check, but I think it won't affect the default behavior as the cstd definition in profile is being discarded by Conan 2.5. Maybe we need to bump the necessary Conan version.

I see that you are also doing some other refactors/improvements. While they look good, I think it is better to separate them to another PR and keep this one focused on the cstd thing exclusively.

@jhol
Copy link
Contributor Author

jhol commented Jul 19, 2024

I see that you are also doing some other refactors/improvements. While they look good, I think it is better to separate them to another PR and keep this one focused on the cstd thing exclusively.

Ok, I've opened another PR with tidy-ups across the board: #656 . I'll come back later with another attempt at adding cstd support when/if these are accepted.

@jhol jhol closed this Jul 19, 2024
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.

2 participants