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

Sort elements by name rather than type #57

Merged
merged 1 commit into from
Sep 24, 2018
Merged

Sort elements by name rather than type #57

merged 1 commit into from
Sep 24, 2018

Conversation

DSteve595
Copy link
Contributor

The fields accessed by the processor have no guaranteed ordering, which I assume is why fields is sorted by type in the first place. In incremental builds, the order seems to change randomly, and this hurts compilation avoidance as then the generated StateSaver classes are different (calls shifted around but functionally not changing).
Since fields is being sorted by type, fields of the same type still have no guaranteed ordering. This changes them to be sorted by their name rather than their type. I can't think of any problems with sorting them by name, as field names have to be unique within a class.
Tested in a large project and a sample project. Thanks for the well-organized project, by the way.

The fields accessed by the processor have no guaranteed ordering, which I assume is why `fields` is sorted by type in the first place. In incremental builds, the order seems to change randomly, and this hurts compilation avoidance as then the generated StateSaver classes are different (calls shifted around but functionally not changing).
Since `fields` is being sorted by type, fields of the same type still have no guaranteed ordering. This changes them to be sorted by their name rather than their type. I can't think of any problems with sorting them by name, as field names have to be unique within a class.
Tested in a large project and a sample project. Thanks for the well-organized project, by the way.
@vRallev
Copy link
Contributor

vRallev commented Sep 24, 2018

I'm super sorry for the long delay. I agree that this change makes sense. Thanks for creating the PR!

@vRallev vRallev merged commit 3624d09 into Evernote:master Sep 24, 2018
@DSteve595
Copy link
Contributor Author

No problem, appreciate it! Glad this is still maintained.

@DSteve595 DSteve595 deleted the patch-1 branch September 24, 2018 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants