Avhtuple to android branch #663

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
2 participants
@iArnold
Contributor

iArnold commented Jan 20, 2014

I noticed my pull request was against the master instead of the android.
Another try.

I created a branch of the android branch because that has pair support in it. I worked my way through the changes and copied the inserted lines for the pair support into the basics to support tuple! and save you the time of doing this.

Not tested, and there still is a lot left to do in tuple.reds and the logic in the lexer.r file. ( I did not figure out what the opt [ ] does)

Hope you will accept this start and I hope it did save you a little time.

iArnold added some commits Jan 20, 2014

Preliminary support for tuple! Update boot.red
Already started some trivial coding for tuple! support so that Superman doesn't have to spend his time on this.
Update common.reds
Added z next to the x and y for pair! for tuple! support
Update structures.reds
Added tuple! structure
Update macros.reds
Added TYPE_TUPLE
Update red.reds
Added tuple next to pair
Create tuple.reds
New file. Direct copy of pair.reds
Added myself as author and changed all occurrences of the word pair to tuple.
Needs some work on red-tuple! type.
Update tuple.reds
some minor additions for the 3rd part of the tuple
@dockimbel

This comment has been minimized.

Show comment Hide comment
@dockimbel

dockimbel Jan 22, 2014

Member

Thanks for the try at implementing a new datatype, but there are many issues in your code:

  • Code is untested, that is already enough for me to not accept a PR. If you want to save me time, the least you can do is ensure that the provided code works...:-/
  • The memory model used by your tuple! is wrong (hence most of the code in %tuple.reds is wrong too). Tuple! are using bytes not integers.
  • You lack some understanding of the Red internal core features, like the fixed slot size for values which is 128-bit. Your red-tuple! definition uses 160 bits...
  • Minor issues, but still annoying: you need to respect the whitespaces usage in Red codebase. Your code editor is adding an extra line at the end of all the modified Red files... Also in %lexer.r, the following code is oddly lacking separating whitespaces: |"pair!"|"tuple!"...

Sorry, I can't accept your PR for tuple! in such form...

PS: typing code is the less time-consuming task for me. Implementing the right features and debugging them eat the most of my coding time.

Member

dockimbel commented Jan 22, 2014

Thanks for the try at implementing a new datatype, but there are many issues in your code:

  • Code is untested, that is already enough for me to not accept a PR. If you want to save me time, the least you can do is ensure that the provided code works...:-/
  • The memory model used by your tuple! is wrong (hence most of the code in %tuple.reds is wrong too). Tuple! are using bytes not integers.
  • You lack some understanding of the Red internal core features, like the fixed slot size for values which is 128-bit. Your red-tuple! definition uses 160 bits...
  • Minor issues, but still annoying: you need to respect the whitespaces usage in Red codebase. Your code editor is adding an extra line at the end of all the modified Red files... Also in %lexer.r, the following code is oddly lacking separating whitespaces: |"pair!"|"tuple!"...

Sorry, I can't accept your PR for tuple! in such form...

PS: typing code is the less time-consuming task for me. Implementing the right features and debugging them eat the most of my coding time.

@dockimbel dockimbel closed this Jan 22, 2014

@dockimbel dockimbel referenced this pull request Jan 22, 2014

Closed

Xtratuple1 #665

@iArnold iArnold deleted the iArnold:avhtuple branch Feb 13, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment