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

[CMakeLists.txt] Remove explicit `CMAKE_ANSI_CFLAGS` #5439

Closed
wants to merge 1 commit into from

Conversation

@SamuelMarks
Copy link
Contributor

SamuelMarks commented May 21, 2020

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

SamuelMarks commented May 22, 2020

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

Copy link
Member

Lekensteyn left a comment

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

SamuelMarks commented May 23, 2020

@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?

Copy link
Member

Lekensteyn left a comment

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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.