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

added ⋆ as unary operator #31604

Merged
merged 5 commits into from
Apr 9, 2019
Merged

Conversation

chakravala
Copy link
Contributor

as disccussed in star is not an operator

@MasonProtter
Copy link
Contributor

Can it be a unary-and-binary-op instead? The Moyal Star product is a legit use of this operator. I don't feel strongly about this though, so if there's a good argument to not bloat the list of unary-and-binary-ops feel free to ignore me.

@chakravala
Copy link
Contributor Author

@MasonProtter good point, I just committed the change

@JeffBezanson
Copy link
Member

For this to work as an infix operator it also needs to be added to a precedence level at the top of the file.

@JeffBezanson JeffBezanson added the parser Language parsing and surface syntax label Apr 4, 2019
@chakravala
Copy link
Contributor Author

the prec-times precedence is probably the most appropriate, given that Moyal product is a "product", so that's what I have committed for now

@chakravala
Copy link
Contributor Author

any update on whether could still be merged before v1.2 is frozen?

@StefanKarpinski
Copy link
Member

So you want the unary operator version of this to be prefix not postfix? Isn't the traditional Kleene star notation postfix?

@improbable-22
Copy link

This PR seems to be about \star which was already a binary operator, and is now listed twice in prec-times.

But the discourse thread was mostly about \bigstar ★. Which would also be nice to have.

@chakravala
Copy link
Contributor Author

chakravala commented Apr 4, 2019

Yes, prefix (not postfix) is used for the Hodge star operation that I implemented in Grassmann.jl

julia> using Grassmann

julia> @basis^3
(⟨+++⟩, v, v₁, v₂, v₃, v₁₂, v₁₃, v₂₃, v₁₂₃)

julia> (v1), (v2), (v3)
(v₂₃, -1v₁₃, v₁₂)

@stevengj recommended to use \star instead of \bigstar in the discourse

@MasonProtter
Copy link
Contributor

MasonProtter commented Apr 4, 2019

@improbable22 is right, \star is already a binary operator. My bad.

julia> (x,y) = (x+y)/(x - y)
 (generic function with 1 method)

julia> 1  2
-3.0

@chakravala
Copy link
Contributor Author

chakravala commented Apr 4, 2019

So it should not be added to unary-and-binary-ops and prec-times; while unary-ops is sufficient?

@JeffBezanson
Copy link
Member

Oh, sorry I misunderstood --- the linked discourse post is about adding \bigstar, so I assumed this was adding that new operator.

src/julia-parser.scm Outdated Show resolved Hide resolved
@chakravala
Copy link
Contributor Author

alright, I have reverted the last 2 commits then

@stevengj
Copy link
Member

stevengj commented Apr 4, 2019

I think both and .⋆ still need to be added to unary-and-binary-ops (in addition to unary-ops).

@chakravala
Copy link
Contributor Author

chakravala commented Apr 4, 2019

are you sure? because this does work for me without it

julia> (x) = x
 (generic function with 1 method)

julia> 8
8

julia> (x,y) = x+y
 (generic function with 2 methods)

julia> 78
15

I also get some compile errors when I have unary-and-binary-ops also, so I think it's not needed

Sysimage built. Summary:
Total ───────  83.465780 seconds 
Base: ───────  27.608859 seconds 33.0781%
Stdlibs: ────  55.854563 seconds 66.9191%
    JULIA usr/lib/julia/sys-o.a
Internal error: encountered unexpected error in runtime:
UndefVarError(var=:li)
rec_backtrace at /home/flow/julia-char/julia/src/stackwalk.c:94
jl_throw at /home/flow/julia-char/julia/src/task.c:210
jl_undefined_var_error at /home/flow/julia-char/julia/src/rtutils.c:130
jl_get_binding_or_error at /home/flow/julia-char/julia/src/module.c:290
_uncompressed_ast at ./reflection.jl:906
typeinf_ext at ./compiler/typeinfer.jl:560
typeinf_ext at ./compiler/typeinfer.jl:599
unknown function (ip: 0x7fc1eb33e417)
jl_apply_generic at /home/flow/julia-char/julia/src/gf.c:2191
jl_apply at /home/flow/julia-char/julia/src/julia.h:1604 [inlined]
jl_type_infer at /home/flow/julia-char/julia/src/gf.c:207
jl_compile_method_internal at /home/flow/julia-char/julia/src/gf.c:1773
jl_apply_generic at /home/flow/julia-char/julia/src/gf.c:2196
jl_apply at /home/flow/julia-char/julia/src/julia.h:1604 [inlined]
jl_finish_task at /home/flow/julia-char/julia/src/task.c:167
jl_threadfun at /home/flow/julia-char/julia/src/partr.c:217
start_thread at /usr/lib/libpthread.so.0 (unknown line)
clone at /usr/lib/libc.so.6 (unknown line)
Generating precompile statements...

this error doesn't happen when unary-and-binary-ops is not used

this PR should be okay as-is right now

@JeffBezanson
Copy link
Member

I also get some compile errors when I have unary-and-binary-ops also, so I think it's not needed

That error is definitely not related to this (fix: #31613).

Try [1 ⋆2] and [1 ⋆ 2]; I think those cases are affected by it.

@chakravala
Copy link
Contributor Author

Alright, here is before

julia> [1 2], [1  2]
([3], [3])

and after

julia> [1 2], [1  2]
([1 2], [3])

you are right, this case needs to be resolved, so I have added it to unary-and-binary-ops again

@chakravala
Copy link
Contributor Author

any more comments? is this PR approved?

src/julia-parser.scm Outdated Show resolved Hide resolved
@chakravala
Copy link
Contributor Author

any more comments on the star operation? hoping to have this merged for v1.2

@StefanKarpinski StefanKarpinski merged commit 6e33c95 into JuliaLang:master Apr 9, 2019
chakravala added a commit to chakravala/julia-vim that referenced this pull request Apr 19, 2019
carlobaldassi pushed a commit to JuliaEditorSupport/julia-vim that referenced this pull request Apr 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parser Language parsing and surface syntax
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants