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

Adapted ChartLegendRenderer class to upcoming Swift 3 changes and improved code readability #643

Merged
merged 6 commits into from
Jan 8, 2016

Conversation

zntfdr
Copy link
Contributor

@zntfdr zntfdr commented Jan 2, 2016

Just some small changes under the hood.

@danielgindi
Copy link
Collaborator

I like most of the changes, but, how is this a performance improvement?

screen shot 2016-01-03 at 9 51 15 pm

@pmairoldi
Copy link
Collaborator

Removing c-style loops is probably a good idea since they will be deprecated in swift 2.2 and removed in 3. Is it faster if you remove the var in for in. I'm assuming making it immutable would make it faster.

@zntfdr
Copy link
Contributor Author

zntfdr commented Jan 3, 2016

That is quite a difference! I offer my apologies and will change the title.

C-style loops are going away, along with ++ and -- operators, their replacement is more of future-proof thing than a performance improvement. I wrongly assumed that for-in and C-style loops had comparable performances.

If there are better ways to replace them, please let me know: I will be happy to change/replace that commit.

This doesn't make things much better but, at least on the first cycle, both for-in and C-style loops have similar performance when the upper limit is controlled by multiple variables (sometimes the for-in loop even beats the C-Style one)

screen shot 2016-01-04 at 06 30 18

screen shot 2016-01-04 at 06 44 58

@zntfdr zntfdr changed the title Improved ChartLegendRenderer class performance and code readability Improved ChartLegendRenderer class future-proof and code readability Jan 3, 2016
@zntfdr zntfdr changed the title Improved ChartLegendRenderer class future-proof and code readability Adapted ChartLegendRenderer class to upcoming Swift 3 changes and improved code readability Jan 3, 2016
@liuxuan30
Copy link
Member

defer { CGContextRestoreGState(context) } is it guarenteed to be called at the last line of the same scope/block?

@zntfdr
Copy link
Contributor Author

zntfdr commented Jan 5, 2016

@liuxuan30 Yes, this is exactly the definition of the defer statement.

@liuxuan30
Copy link
Member

@zntfdr thanks! another question, what's the swift 3 changes?

@zntfdr
Copy link
Contributor Author

zntfdr commented Jan 5, 2016

@liuxuan30 sure! No problem :)

As for the Swift 3 changes, please refer to these two comments.

If you're interested on knowing more about the upcoming Swift (major) changes, I'm happy to invite you to visit this official Swift Evolution Repository.

@danielgindi
Copy link
Collaborator

Actually I tend not to have multiple conditions if I can do a "max" or a "min" ahead of time and store in a variable.

But that's a "moo point" as they will remove C style loops. Which is weird. Why would they do that? One might want to add another condition which is not on the same index...
Sure there are other ways to loop, like while, and other ways of controlling a loop like continue and break, but I can't see why remove support from Swift for C style loops.

danielgindi added a commit that referenced this pull request Jan 8, 2016
Adapted ChartLegendRenderer class to upcoming Swift 3 changes and improved code readability
@danielgindi danielgindi merged commit 35379a2 into ChartsOrg:master Jan 8, 2016
@zntfdr
Copy link
Contributor Author

zntfdr commented Jan 8, 2016

These are the official reasons.

Regarding the performance concerns, I'd invite you to read this fierce discussion that went on during the proposal review (the performance issue starts here).

TL;DR:
Performance (among different loops) greatly differ based on what's inside the loops, which build you're using, optimization levels, and other factors.

If performance is a major factor here, the best solution is testing different Swift loop styles every time one is needed and use the best accordingly (I don't believe that this is a priority at this state of the framework development).

@liuxuan30
Copy link
Member

I feel sorry that for loop is going to be removed :( Is there a vote I can protest
lots common concept as C like struct, class, but they decide to remove for loop first

@liuxuan30
Copy link
Member

I just recall we have code like for (var i = _minX, maxX = min(_maxX + 1, _xAxis.values.count); i < maxX; i += _xAxis.axisLabelModulus)

If c-style for loop is dropped in swift 3, we need to convert those. I am still surprised the reviews are not having a consistent opinion but they accept this idea.

@danielgindi
Copy link
Collaborator

Yes I'm not really arguing about performance here, as that's not where an addition 0.0001 ms would make a difference.

If there's a performance critical loop on that "realtime" level- I would just revert to a while loop.

It seems like the main reason why for loops where removed is that the += operator is not returning a value in Swift, which is inconsistent with ++ operator which does return a value. And the fact that they are both derived from C, they decided to just remove them. And so goes the for loop also, because in most of those loops there's usage of the ++ operator.

I'm guessing that the behaviour of the +=/++ operators could not be fixed to something consistent because of the way Swift works with values, and maybe because the way the optimizer works. I think it is worth trying to just work on that instead of removing that chunk of code from the compiler, but my opinion does not matter here as the change as already been accepted.

@pmairoldi
Copy link
Collaborator

I disagree. I like the change. C style for loops had to be mutating by default which isn't very swift like. You can still achieve the most of the same things with for in but you have to think more swifty. An example mentioned in the mailing list is iterating with 2 values. It translates to this:

for (i, j) in zip(10.stride(to: 0, by: -1), 20.stride(to: 0, by: -2)) {
    if i % 2 == 0 { continue }
    print(i, j)
}

This is more swift like and doesn't expose state by having to keep track of incrementing variables. I like this more personally but I hate using var so I am biased.

@danielgindi
Copy link
Collaborator

As I understand it, the point of Swift is to concentrate on functionality and not on syntax and code wizardy, to make code more readable and understandable etc.

But I feel like sometimes the new Swift tricks are worse in readability terms than plain old C style.

In the case of C style loops- only time will tell... Let's see what happens in 4.0 :)
I was amused by Apple adding the # to shorten argument declarations, and then reverting to specifying the name twice, they decided that it is more readable.
They do not care much that they break code, they only care about making their "product" perfect. This allows for a faster language evolution.
So if removing C loops and operators will prove to be the wrong move- they will get it back. If not, we will all know it ;)

@pmairoldi
Copy link
Collaborator

We will see indeed. I'm just happy to work with a language that I really like 😀.

@liuxuan30
Copy link
Member

for (i, j) in zip(10.stride(to: 0, by: -1), 20.stride(to: 0, by: -2)) is really killing me 😢

@danielgindi
Copy link
Collaborator

@liuxuan30 yes that's understandable...

The people proposing such changes have the claim that "someone coming from a non-C language will have a steeper learning curve" as an excuse to do something weird or to remove C-like syntax, and come up with an even steeper learning curve.
I suspect its just plain old antiCism ;-)

@zntfdr
Copy link
Contributor Author

zntfdr commented Jan 11, 2016

@danielgindi as a C developer, I completely agree.

If people really want to write C-style loops, it's easy to integrate a C class inside a Swift framework.

Syntactically, Swift is completely different than C, and the C-Style loop was a black sheep in this new language.

@danielgindi
Copy link
Collaborator

@zntfdr I agree - as of the moment. But in the past, when Swift was founded, it was meant to be completely interoperable and compatible with C, C++ and ObjC.
What really happened is they realized the power of Swift, and complete interoperability is not a priority anymore - as you can see in the discussions about Swift 3.

So I really get why C loops would be weird in Swift, but those arguments that C style implies a steeper learning curve - those are just ridiculous.

A few weeks ago, I recall I stumbled upon a post in SO, where someone recommended using i = i + 1 instead of i++, claiming that someone coming from a non-C language will have a harder time reading the code. And they I think "Hey... isn't this code C? Why would we try to make it readable to someone who doesn't know C?"

So coming back to Swift- if they now state that C/C++ compatibility is not a priority anymore - it's completely acceptable to remove C style code. And we might even see them removing the ; statement-ending semicolons in the near future... And maybe the {} block brackets...
It may turn into Ruby or Python, and then they may decide that Python compatibility isn't a priority and go in another direction... LOL

@pmairoldi
Copy link
Collaborator

I think it's a good idea to evolve from the concepts of c. Even languages looking to replace c like rust have this kind of iteratior based for loop instead. I like the explanation the rust docs give about their for loops

@zntfdr
Copy link
Contributor Author

zntfdr commented Feb 4, 2016

I'm sorry to bring this discussion up again:
for your future use and reference regarding the replacement of C-Style loops, I'd suggest you to read this short article (I have no relations with the author, I just thought it might be useful to you all).

Cheers!

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

4 participants