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

osutils macros: @unix, @osx, etc. #4233

Closed
StefanKarpinski opened this issue Sep 9, 2013 · 15 comments
Closed

osutils macros: @unix, @osx, etc. #4233

StefanKarpinski opened this issue Sep 9, 2013 · 15 comments
Assignees
Labels
kind:breaking This change will break code needs decision A decision on this change is needed
Milestone

Comments

@StefanKarpinski
Copy link
Sponsor Member

The stuff in base/osutils.jl that implements

@osx? 1 : 2

strikes me as a bit too cute. It took me a minute screwing around with the syntax to figure out what was even going on with this macro. Cute as the resulting usage may be, this strikes me as an abuse of syntax. If we do keep it, the documentation needs to be improved to include how you actually call these things and ideally, it would tell you the same information when you inevitably use it incorrectly.

Also, why does the implementation do syntax structure checking at run-time? That should be done during macro expansion.

cc: @vtjnash, @loladiro, @johnmyleswhite (people who touched this file).

@JeffBezanson
Copy link
Sponsor Member

It is using a macro to write the bodies of the macros. _os_test could be a function instead.

@StefanKarpinski
Copy link
Sponsor Member Author

Ah, I see. Yes, that's a bit confusing.

The thing I really don't like about this "fake ternary" syntax punning is how brittle it is:

julia> @osx? 1 : 2
1

julia> @osx? 1:2
1

julia> @osx? 1 :2
ERROR: wrong number of arguments

julia> @osx? 1: 2
1

julia> @osx?1:2
1

julia> @osx? 1:2
1

julia> @osx?1 :2
ERROR: wrong number of arguments

julia> @osx?1: 2
1

julia> @osx?1 : 2
1

julia> @osx?
ERROR: wrong number of arguments

julia> @osx ? 1 : 2
1

julia> @osx ?1:2
1

julia> @osx ?
ERROR: wrong number of arguments

julia> @osx ? 1 :
ERROR: wrong number of arguments

julia> @osx ? 1
ERROR: assertion failed: :(isa(ex,Expr))

Most of these syntax variations work for actual ternary operator expressions but half of them fail here because this isn't really a ternary operator even though it's supposed to look like one.

A couple of alternative options:

  • One-argument macros taking an actual ternary expression: @system osx ? 1 : 2.
  • Zero-argument boolean macros: @osx ? 1 : 2 => true ? 1 : 2; just rely on compiler to specialize on the boolean literal to avoid overhead.
  • Two-or-three argument macros: @osx 1 2 and @osx 1 is equivalent to @osx_only 1, which we can deprecate.

I lean towards the last option myself, since it's the simplest and reduces the number of things in base.

@JeffBezanson
Copy link
Sponsor Member

+1 for combining them into one set of macros

@vtjnash
Copy link
Sponsor Member

vtjnash commented Sep 9, 2013

Other than a better error message for that last expression (or allow it), I don't see how the rest are valid -- they look to me like incomplete expressions.

@StefanKarpinski
Copy link
Sponsor Member Author

No kidding they're incomplete. The point is that the REPL doesn't let me finish the input because it has no idea what it's parsing.

@StefanKarpinski
Copy link
Sponsor Member Author

In other words, this "syntax" is absurdly sensitive to whitespace – in particular line breaks confuse the hell out of it.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Sep 9, 2013

Perhaps
@os windows ? 1 : 2
And
@os windows 3

Parentheses and begin/end are always helpful for the compiler -- and reviewer -- when you want to split over a newline.

@StefanKarpinski
Copy link
Sponsor Member Author

You're missing the point that this looks like a ternary operator so the user expects it to behave like one syntactically – but it isn't one at all and doesn't behave like one syntactically at all and instead breaks in all sorts of ways that ternary operators don't. If you just stop trying to make it look cute and make it what it is – individual expressions passed to a macro – then all the confusion goes away. Yes, it looks slightly less pretty, but it's much more usable.

@JeffBezanson
Copy link
Sponsor Member

@os windows ? 1 : 2 does parse the macro argument like a normal ternary operator.

@StefanKarpinski
Copy link
Sponsor Member Author

Yes, that's why I proposed it since it actually has the syntax it looks like it has. And that does reduce all of these things to a single macro which is rather nice.

@aviks
Copy link
Member

aviks commented May 3, 2015

A few more issues with this, for the record.

  1. The behaviour has changed between 0.3 and 0.4 . In 0.4, a space after the ? is mandatory.

In v0.3

julia> @osx? print("A"):print("B")
A

In v0.4

julia> @osx?print("!"):print("B")
ERROR: wrong number of arguments

julia> @osx? print("A"):print("B")
A
  1. Error location information is missing for parsing syntax errors for this macro. Although, is this true of all macros?
julia> function f()
          println(1)
          println(2)
          @osx?print("Y"):print("N")
       end
ERROR: wrong number of arguments

@vtjnash
Copy link
Sponsor Member

vtjnash commented May 3, 2015

OT: the parser is fine with this expression, the error happens later when starting to evaluate it.

@tkelman
Copy link
Contributor

tkelman commented Apr 21, 2016

I said this in a different issue but can't find it right now, this fake ternary would be better replaced by a conditional macro @if or @cond for the cases where this os-dependent code selection has to happen at parse time.

@StefanKarpinski
Copy link
Sponsor Member Author

+1 to @if

@tkelman
Copy link
Contributor

tkelman commented May 5, 2016

@vtjnash also suggests the possibility of a @static modifier, so we could do @static if - or something similar to force parse-time evaluation of a pure condition

vtjnash added a commit that referenced this issue May 5, 2016
vtjnash added a commit that referenced this issue May 5, 2016
vtjnash added a commit that referenced this issue May 5, 2016
vtjnash added a commit to vtjnash/julia that referenced this issue May 6, 2016
vtjnash added a commit that referenced this issue May 13, 2016
vtjnash added a commit that referenced this issue May 16, 2016
vtjnash added a commit that referenced this issue May 17, 2016
implements #5892
closes #6674 and #4233

Sys.KERNEL now replaces OS_NAME and unambiguously returns
the name of the kernel reported by uname for the build system configuration
vtjnash added a commit that referenced this issue May 17, 2016
implements #5892
closes #6674 and #4233

Sys.KERNEL now replaces OS_NAME and unambiguously returns
the name of the kernel reported by uname for the build system configuration
@vtjnash vtjnash self-assigned this May 18, 2016
vtjnash added a commit that referenced this issue May 20, 2016
implements #5892
closes #6674 and #4233

Sys.KERNEL now replaces OS_NAME and unambiguously returns
the name of the kernel reported by uname for the build system configuration
vtjnash added a commit that referenced this issue May 20, 2016
implements #5892
closes #6674 and #4233

Sys.KERNEL now replaces OS_NAME and unambiguously returns
the name of the kernel reported by uname for the build system configuration
vtjnash added a commit that referenced this issue May 20, 2016
implements #5892
closes #6674 and #4233

Sys.KERNEL now replaces OS_NAME and unambiguously returns
the name of the kernel reported by uname for the build system configuration
vtjnash added a commit that referenced this issue May 21, 2016
implements #5892
closes #6674 and #4233

Sys.KERNEL now replaces OS_NAME and unambiguously returns
the name of the kernel reported by uname for the build system configuration
vtjnash added a commit that referenced this issue May 21, 2016
implements #5892
closes #6674 and #4233

Sys.KERNEL now replaces OS_NAME and unambiguously returns
the name of the kernel reported by uname for the build system configuration
@vtjnash vtjnash closed this as completed May 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:breaking This change will break code needs decision A decision on this change is needed
Projects
None yet
Development

No branches or pull requests

5 participants