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

Undeprecate {os}_only macros #16658

Closed
wants to merge 1 commit into from
Closed

Conversation

kmsquire
Copy link
Member

I kind of feel that #16219 threw the os_only baby out with the os bathwater, so-to-speak. I think it's great having a macro which statically evaluates items at top scope, but feel that the readability of code using the @{os}_only macros went down dramatically when they were deprecated. So I'd like to reintroduce them based on the @static macro, with a few small changes:

Thoughts/opinions?

* These were convenient and less noisy than the current
  {@}static if is_{os} counterpart introduced in #16219
* {@}osx_only is now {@}apple_only, to match the change in #16219
* added {@}bsd_only
@tkelman
Copy link
Contributor

tkelman commented May 30, 2016

Should be put back in the rst docs if we decide to add them back.

Now that the work was already done (in Base anyway) I'm in favor of leaving these deprecated since @static is strictly more general, and the main thing you lose is needing to write the end. I just didn't think it was as urgent to remove them immediately, and #16219 could have been a much smaller PR if they hadn't been.

@tkelman tkelman added the needs decision A decision on this change is needed label May 30, 2016
@kmsquire
Copy link
Member Author

Yep, I agree that @static is strictly more general, and think it is a good tool. But I think that, for OS-specific code, it's less readable and less obvious.

For readability, I personally find this (in julia 0.4) much clearer than this (after #16219).

Re: less obvious: if I'm writing code that needs to be operating system specific, I'll certainly find and use the is_windows(), is_apple(), etc. functions, but it's unlikely that I'll figure out that I should use @static when I'm not working in global scope (or understand that I should). (Well, I'll probably figure it out, but I don't think your average user would).

Anyway, if this is acceptable, it should be merged soon. I'll give it a few days, and if there's no decision made to merge it, I'll put it in a package (and try to figure out how to interact with deprecated macros in Base).

@kmsquire
Copy link
Member Author

(Oh, and if this is going to be merged, I'll update the rst docs.)

@vtjnash
Copy link
Member

vtjnash commented May 30, 2016

Re: less obvious: if I'm writing code that needs to be operating system specific, I'll certainly find and use the is_windows(), is_apple(), etc. functions, but it's unlikely that I'll figure out that I should use @static when I'm not working in global scope (or understand that I should).

You should never really need @static, it can simply help reduce code image size / duplication when not doing a cross-compile. I'm also hoping that windows will someday become a subset of @unix (ref the ongoing work at http://midipix.org), so I'm not sure it's best to assume that @unix_only and @windows_only are mutually-exclusive binary options.

statically evaluates items at top scope

you do realize that statement is redundant, right? the @static macro exists to evaluates non-toplevel conditionals at the top-level/static scope.

@tkelman
Copy link
Contributor

tkelman commented May 30, 2016

Wishful thinking aside, currently they often are mutually exclusive, and I think that's clearer when expressed as an if-else. Static could use significantly more descriptive documentation explaining when it is or is not necessary though.

@kmsquire
Copy link
Member Author

you do realize that statement is redundant, right? the @static macro exists to evaluates non-toplevel conditionals at the top-level/static scope.

Yep, I did realize this, and no, I didn't state it very clearly. But what is unclear to me is the difference between top-level and global scope. For example, if I'm defining a function inside a module, I would guess that I'm not at top level scope, and therefore, for all practical reasons, I would need to use static. But I'm not sure enough about this.

Either way, that's getting away from the main thrust of this PR, which is whether or not {os}_only should exist as macros or not. @tkelman thinks not. I think so. Any other opinions?

@vtjnash
Copy link
Member

vtjnash commented May 31, 2016

difference between top-level and global scope

there's no difference

inside a module

everything is in a module

@kmsquire
Copy link
Member Author

inside a module

everything is in a module

True. I was confused about "global scope" not extending beyond module boundaries. My bad.

Coming back to this PR: after thinking about @tkelman's last response, I guess I can see that using if is_{os}() might generally provide better flow (even if I think that @windows_only, etc. looks cleaner), especially since, with the current macros in this PR, it's relatively easy to specify @windows_only and @unix_only, but harder to express, e.g., @windows_only, @apple_only, and everything else (Linux + the remaining (non-darwin) BSDs).

So the if_{os}() functions currently in Base do give the most flexibility.

@omus
Copy link
Member

omus commented May 31, 2016

I preferred the {os}_only macros when creating OS specific methods. Overall I think the is_{os}() functions are a good change especially for ternary operator.

@nalimilan
Copy link
Member

The macros are convenient for simple cases, but I also think that the if system is cleaner and more robust. It would probably be better to only use the standard if syntax everywhere (and thus keep the macros deprecated).

@vtjnash vtjnash closed this Jun 2, 2016
@ararslan ararslan deleted the kms/undeprecate_os_only_macros branch June 4, 2017 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs decision A decision on this change is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants