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

Fix c(pp)dialect generating uppercase C(++)xx for Xcode, replace gnu99 with compiler-default #897

Merged
merged 3 commits into from
Oct 2, 2017

Conversation

hsandt
Copy link
Contributor

@hsandt hsandt commented Sep 20, 2017

As mentioned in #895, when using cdialect "C98", "C90", etc. (resp. cppdialect "C++98", "C++11", etc.), the build configuration for Xcode uses GCC_C_LANGUAGE_STANDARD = C98, C90, etc. (resp. CLANG_CXX_LANGUAGE_STANDARD = "C++98", "C++11", etc.) with uppercase, which produces in turn a command line option "std=C98", "std=C90", etc. which are considered invalid values by clang.

The pull request contains 2 parts:

  1. Fixes the problem above by associating an explicit string for each dialect (compiler-default, c++ and gnu). Note that the behavior for gnu should not have changed.
  2. Not using cdialect now doesn't set GCC_C_LANGUAGE_STANDARD (Xcode will use an implicit compiler-default), while using cdialect "Default" will explicitly set GCC_C_LANGUAGE_STANDARD to "compiler-default" (which correspond to gnu11 today). Previously, gnu99 was used with cdialect "Default" or no cdialect at all.

I also added the corresponding unit tests for Xcode. For the 2nd part, I had to modify existing tests by removing lines with gnu99.

Since the 2nd part actually changes the previous behavior and may break configuration on existing projects, I would like you to discuss whether I should keep it or not. If you only want the 1st part, I will add gnu99 as a fallback when cdialect is not used. However, I still think that it makes sense to use "compiler-default" when cdialect or cppdialect is explicitly used with "Default".

Copy link
Member

@samsinsane samsinsane left a comment

Choose a reason for hiding this comment

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

I've made a request for a change, but I'm happy for this to be merged as-is since it's currently broken.

return
end

local cLanguageStandards = {
Copy link
Member

Choose a reason for hiding this comment

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

It would be kind of nice to have these exist outside of the function. It makes extending or modifying them much easier instead of overriding an entire function, you just xcode.cLanguageStandards["C20"] = "c20".

@hsandt
Copy link
Contributor Author

hsandt commented Sep 22, 2017

All right, I'll try it. Separating data from functions seems good practice to me, and it would also solve the issue of having repeated heap allocations (although that's probably a minor issue, considering our purpose).

@hsandt
Copy link
Contributor Author

hsandt commented Sep 22, 2017

I've done it. I also made the functions local since we do not access them from outside. Note that if we want to test them later, we'll have to make them part of the xcode module again. For now, the tests on XCBuildConfiguration_Target cover what setCLanguageStandard and setCppLanguageStandard do, so I don't think that's necessary. That said, in pure unit test practice, testing the functions independently would normally be the best.

@samsinsane
Copy link
Member

I'm not a fan of local functions, this module is hard enough to override without having to copy even more functions to make it even remotely possible to override properly.

@hsandt hsandt force-pushed the dialect-xcode branch 2 times, most recently from 438e940 to 520a61f Compare September 29, 2017 19:50
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.

3 participants