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

Tweaks for 1.2b3 #5

Closed
wants to merge 7 commits into from
Closed

Conversation

robrix
Copy link
Contributor

@robrix robrix commented Mar 21, 2015

Some tweaks for Swift 1.2b3 support, needed by Carthage/Carthage#368.

@jdhealy
Copy link
Owner

jdhealy commented Mar 21, 2015

Thanks for the pull request, Rob! ✨

You might have missed the swift-1.2 branch?

I meant to post about that in Carthage/Carthage#368, apologies 😑


5c8411d and d36ac13 have counterparts there, which allow PrettyColors to compile under Xcode 6.3 beta 3.

As far as the other commits, no objections — just curious, why are the the deployment targets now necessary: change in Xcode and build system, change in Carthage, other?

Also, would you mind explaining GCC_NO_COMMON_BLOCKS a bit? ❤️

@robrix
Copy link
Contributor Author

robrix commented Mar 21, 2015

I did indeed miss that branch!

Deployment target changes are necessary due to Xcode changes. It didn’t enforce them before (it ought to have).

GCC_NO_COMMON_BLOCKS was suggested by Xcode and I get tired of staring at the ⚠️ icon.

@jdhealy
Copy link
Owner

jdhealy commented Mar 21, 2015

First thing I found about GCC_NO_COMMON_BLOCKS was this Stack Overflow answer — it quotes:

In C, allocate even uninitialized global variables in the data section of the object file, rather than generating them as common blocks. This has the effect that if the same variable is declared (without extern ) in two different compilations, you will get an error when you link them. The only reason this might be useful is if you wish to verify that the program will work on other systems which always work this way.

I have no idea why this would be necessary in Swift with namespaces, but I’m cool with adding it just to shut Xcode up.


A couple git best practices questions:

  1. Should I cherry-pick those commits before 5c8411d or would you prefer to open a new pull request for the the swift-1.2 branch?
  2. It would be nice if the messages of the commits were a little more precise (“bump”ing what, “recommend”ing why, “target”ing which). If I was hypothetically to cherry-pick the commits, would it be poor etiquette to amend the commits myself (aka is commit authorship indicative of both the message and the code of the commit) ❓

@robrix
Copy link
Contributor Author

robrix commented Mar 21, 2015

@jdhealy Honestly, whatever you prefer is fine by me. I have zero concern beyond unblocking Carthage.

@jdhealy
Copy link
Owner

jdhealy commented Mar 21, 2015

@robrix Okay, thanks for your time and effort.

If all's fine with you, I'll make these same commits on the swift-1.2 branch and credit you in the commit messages. 😃

jdhealy pushed a commit that referenced this pull request Mar 21, 2015
Credit to @robrix. Commit message amended by @jdhealy. See <#5>.
jdhealy pushed a commit that referenced this pull request Mar 21, 2015
Shouldn't effectively make a difference in the real world, since there's no C code in the library, but this eliminates an warning in Xcode.

Credit to @robrix. Commit message amended by @jdhealy. See <#5>.
jdhealy pushed a commit that referenced this pull request Mar 21, 2015
Credit to @robrix. Commit message amended by @jdhealy. See <#5>.
jdhealy pushed a commit that referenced this pull request Mar 21, 2015
Credit to @robrix. Commit message amended by @jdhealy. See <#5>.
jdhealy pushed a commit that referenced this pull request Mar 21, 2015
Credit to @robrix. Commit message amended by @jdhealy. See <#5>.
@jdhealy
Copy link
Owner

jdhealy commented Mar 21, 2015

Cherry-picked the commits and amended their messages.

Appended the following to the end of the messages:

Credit to @robrix. Commit message amended by @jdhealy. See <https://github.com/jdhealy/PrettyColors/pull/5>.

Closing this pull request.

Thanks again Rob!

@jdhealy jdhealy closed this Mar 21, 2015
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.

None yet

2 participants