Great Code

[janus]

I just checked out your code, pretty good. But "switch " statement would be my choice if I am to match single length string. I noticed that you didn't check for zero denomination before division.

[soveran]

Hello @janus, thanks a lot for reviewing the code!

Sadly, I can't use a switch statement because I need to match the whole word, not only the first character. Given the fact that I use dynamic strings, I could add an optimization that checks the size of the string in constant time (a feature of the sds library) and then switch on the first character if the length is 1, with the downside that the code would be more obscure. Given the fact that performance is extremely good as it is, maybe there's no need for that change. But thanks to your comment, I took a second look at those string comparisons and reordered them so that the most frequent check (and likely the fastest) is now at the top.

Regarding division by zero: when I started building the project I used a stack of ints and I had guards for division by zero. Then I moved to doubles and I removed the checks because for floating point arithmetic, division by zero results in inf. Other operations that are invalid with integers may return nan with doubles. When clac pushes those values to the stack, it's very easy to understand what's going on: the inf or nan not only let you operate with them, but also serve as error messages and let the user can handle the situation gracefully.

[janus]

I have not checked your tokenizer but I would assume that your string object has length field already. So checking that out is trivial . You argument on division looks okay.

May be we should be bouncing ideas off another in order to learn .

On Apr 14, 2017 10:01 AM, "Michel Martens" notifications@github.com wrote:

Hello @janus https://github.com/janus, thanks a lot for reviewing the code!

Sadly, I can't use a switch statement because I need to match the whole word, not only the first character. Given the fact that I use dynamic strings, I could add an optimization that checks the size of the string in constant time (a feature of the sds library) and then switch on the first character if the length is 1, with the downside that the code would be more obscure. Given the fact that performance is extremely good as it is, maybe there's no need for that change. But thanks to your comment, I took a second look at those string comparisons and reordered them so that the most frequent check (and likely the fastest) is now at the top.

Regarding division by zero: when I started building the project I used a stack of ints and I had guards for division by zero. Then I moved to doubles and I removed the checks because for floating point arithmetic, division by zero results in inf. Other operations that are invalid with integers may return nan with doubles. When clac pushes those values to the stack, it's very easy to understand what's going on: the inf or nan not only let you operate with them, but also serve as error messages and let the user can handle the situation gracefully.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/soveran/clac/issues/1#issuecomment-294120493, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAaRrjx6oPdhrQ9DosJm5L2zsp-ujJoks5rvzX2gaJpZM4M9MD_ .

[soveran]

Closing this issue for now. Thanks for your help :-)

[Capaverde]

Maybe you could hash the word into an integer and use that value for the switch? Unsure how to do that, though.