Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Update kdb/tick Syntax to Latest Versions (Specifically 4.1) #1
base: master
Are you sure you want to change the base?
Update kdb/tick Syntax to Latest Versions (Specifically 4.1) #1
Changes from 1 commit
e248a6a
8062945
b88ea31
abd7f31
e8b0a4d
d93befc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are not getting a performance bonus from the new implementation I think I would revert back to its previous version, since it's a bit confusing. When reading the code, you look for a conditional statement, but you get something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the main reason for this change was because it's shorter :)
I still believe it's quite clear, so I'll keep it until our master @neutropolis makes the decision
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then, just to drag my point across, should we change all
if
s to this syntax?I.e:
Haven't tested it yet, but I'll do so whenever I get to download v4.1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not new in 4.1. We are simply indexing. For example, in this case, x~ will return 0b or 1b. The idea is to index and return the first or second position from the list on the right. I believe this is suitable for small and simple cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point still stands. I personally would keep it as it was, but if we are changing it then we should keep it consistent and change every instance of a conditional statement OR every if/else (
$
) statement. Let's wait to see what @neutropolis has to say about this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose to add
at the end of the condition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I have been able to guess, since the ifs' are switched, you need to check if
x
is the null symbol on the first condition as well, and this can be achieved usingin
, like it has been done here. Still, I don't really see the point of doing this unless we get a speed benefit of some sort.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I just added it because I tried to incorporate the new syntax as much as possible. I need to measure how fast or slow this is, so once I do that, I'll determine if it's worthwhile or not