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

Introduce EXPR_ELM_MAT_LEV and simplify EXPR_ELMS_LIST_LEV #5451

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zickgraf
Copy link
Contributor

Since 2e850b0, at most two indices in "[]" are allowed. Handling the two possible cases explicitly allows to simplify some parts of the code and makes one's life easier when manipulating syntax trees. However, this obviously is a breaking change:

Previously, the component pos of EXPR_ELMS_LIST_LEV was a list with one or two entries. Now we have to cases:

  • In the case with one entry, pos now simply is this entry.
  • In the case with two entries, the type is now EXPR_ELM_MAT_LEV with components row and col.

I have not yet added tests and a text for release notes because I first want to make sure that you are okay with this breaking change.

Text for release notes

TODO

@ChrisJefferson
Copy link
Contributor

I'm not opposed to this. One question is if we think, at a later date, we might want to add n-dimensional (in which case it might be easiest if everything is in one type? even then we might want to seperate out 1 and 2 dimensional, as the amount of code duplication isn't too much, and this is low-level stuff)

I'm happy with this, but will see if @fingolfin has an opinion (not sure if this might interact with matrix-related stuff).

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine by me

src/vars.c Outdated Show resolved Hide resolved
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, to clarify: fine by me, but merge of course only after tests are adjusted etc. ;-)

@zickgraf
Copy link
Contributor Author

Thanks for the feedback @ChrisJefferson and @fingolfin! Then I will continue with this and maybe also look at the situation for assignments while I'm at it. But first #5453, which fixes the problem which actually made me dig into all of this :D

Since 2e850b0, at most two indices in "[]"
are allowed. Handling the two possible cases explicitly allows to simplify
some parts of the code and makes one's life easier when manipulating syntax
trees. However, this obviously is a breaking change.
@zickgraf
Copy link
Contributor Author

I'm not convinced by this anymore because EXPR_ELM_MAT itself already is a "level" thing:
The x in
[[1,2]]{[1]}[x]
and the x in
[[1,2]][1,x]
work on the same level. Hence, I think the proper solution would be to unify both systems, for example by allowing the syntax [[1,2]][[1],x] as in Julia.

However, this would be a much larger goal then the original intent of this PR and I don't really need it. Thus, I will push a last fixup to make the PR compatible with #5453 and then turn it into a draft in case this comes up again in the future. Feel free to close the PR if you want to reduce the number of "probably never to be finished" drafts.

Subjective additional idea: The Julia syntax could even replace the syntax [[1,2]]{[1]}[x] completely. I think this would be a good idea because at least for me the behaviour of [[1,2]]{[1]}[x] was quite surprising and unexpected when I encountered it the first time.

@zickgraf zickgraf marked this pull request as draft June 12, 2023 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants