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

Jump to bottom

[CMakeLists.txt] Remove explicit CMAKE_ANSI_CFLAGS #5439

Closed
wants to merge 1 commit into from
Closed

[CMakeLists.txt] Remove explicit CMAKE_ANSI_CFLAGS #5439

wants to merge 1 commit into from

Conversation

SamuelMarks
Copy link
Contributor

Hadn't seen the CMAKE_ANSI_CFLAGS flag before, searched it and found: https://gitlab.kitware.com/cmake/cmake/commit/5a834b0bb0bc2889bb67bdaac37ce9b17d4f0f59

@bagder
Copy link
Member

bagder commented May 21, 2020

Can you please clarify why you want to remove this and what effects it will have on users on new and old cmake verisons? That link didn't help me.

@bagder bagder added the cmake label May 21, 2020
@SamuelMarks
Copy link
Contributor Author

That link makes me think that this approach has been redundant for 12 years…

Lekensteyn
Lekensteyn reviewed May 22, 2020
View reviewed changes
Copy link
Contributor

@Lekensteyn Lekensteyn left a comment

Choose a reason for hiding this comment

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

I can confirm that that variable was removed in the referenced commit. A later CMake commit removes the variable from the tests, claiming that it was removed in CMake 2.6. With an updated commit message that clarifies why it is safe (a reference to the CMake commit) to remove I think it is good to take it.

I am curious though how you ran into this piece of code?

@SamuelMarks
Copy link
Contributor Author

@Lekensteyn Great!

Oh, I just searched for CMAKE_ANSI_CFLAGS when I was reviewing libcurl's way of doing things in its CMakeLists.txt (for my own projects, I'm adding support for different crypto libraries, HTTP & HTTPS libraries, &etc.).

@bagder
Copy link
Member

bagder commented Aug 12, 2020

@Lekensteyn is that an ack from you to merge this?

Lekensteyn
Lekensteyn approved these changes Aug 18, 2020
View reviewed changes
Copy link
Contributor

@Lekensteyn Lekensteyn left a comment

Choose a reason for hiding this comment

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

Yes sure, the patch looks fine. I would suggest updating the commit message to make clear why it is safe to remove this, but it looks fine otherwise.

@bagder bagder closed this in d541f83 Sep 4, 2020
@bagder
Copy link
Member

bagder commented Sep 4, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lekensteyn Lekensteyn Lekensteyn approved these changes

Assignees
No one assigned
Labels
cmake
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@SamuelMarks @bagder @Lekensteyn