Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Date and "empty objects" that are not objects. #35

Merged
merged 1 commit into from
Nov 19, 2015

Conversation

joar
Copy link
Contributor

@joar joar commented Nov 17, 2015

Add support for Date objects and "empty objects" that are not objects.
Uses obj.toString() to display the object.

Also convert "CONSTANTS" to be bitmask-compatible and replace switch statement with if statement in order to be able to use bitmasks.

Add support for Date objects and "empty objects" that are not objects.
Uses obj.toString() to display the object.
@marianoguerra
Copy link
Owner

hi, thanks for your patch!

just some observations, did you changed to ifs only to use bitmasks? why not just introduce another constant for special objects and keep the switch?

I'm using this lib in a performance sensitive place (hundreds of potentially big objects) and when I update this lib, if my benchmarks show that the if is slower I will revert back to a switch statement.

just want to know if there's a performance reason that I don't know of (I would expect switch to be optimized to a jump table but the "if" solution not, but maybe there's an optimization I'm not aware of)

@joar
Copy link
Contributor Author

joar commented Nov 19, 2015

I chose if..else-if..else because it was more sraight-forward when dealing with bitmasks.

I've put together a perf test which compares the isolated bitmask-in-switch and bitmask-in-if behaviour and the difference is minimal.

marianoguerra added a commit that referenced this pull request Nov 19, 2015
Date and "empty objects" that are not objects.
@marianoguerra marianoguerra merged commit bf42723 into marianoguerra:master Nov 19, 2015
@marianoguerra
Copy link
Owner

ok, thanks for the perf test and the patch!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants