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

Make math-mode=fast no op, it's too dangerous #41638

Merged
merged 4 commits into from
Apr 14, 2022

Conversation

PallHaraldsson
Copy link
Contributor

Fixes: #25028

Competing PR to my other #41607

@KristofferC
Copy link
Member

I don't think it is worth opening a bunch of different variation PRs about this. The difficulty here isn't really to just remove the relevant lines, it is to come to a conclusion. When that has been done, the PR can be updated to whatever that conclusion is. Having multiple versions of a PR just risks splitting the discussion making it harder to follow the conversation.

@PallHaraldsson
Copy link
Contributor Author

PallHaraldsson commented Jul 19, 2021

Ok, let's keep the discussion at the other PR. I was trying to address your concern that that other PR was a breaking change (while only for the command-line option, which was the point of that PR).

@PallHaraldsson
Copy link
Contributor Author

PallHaraldsson commented Aug 12, 2021

@JeffBezanson, if I understood you correctly at the other PR, then according to triage, this one should be merged (or possibly without the "added "broken" option").

@PallHaraldsson
Copy link
Contributor Author

Bump. It seemed like Jeff approved the way this PR works in #41607 (comment)

@oscardssmith
Copy link
Member

As I read it, the PR currently deletes the option rather than make it a no-op.

@PallHaraldsson
Copy link
Contributor Author

PallHaraldsson commented Sep 7, 2021

No, it keeps the --math-mode=fast, just disables it, while, yes, dropping (only for now, not other docs) from --help. It also adds undocumented "broken" option.

@oscardssmith oscardssmith added the triage This should be discussed on a triage call label Mar 30, 2022
@oscardssmith oscardssmith reopened this Mar 30, 2022
@PallHaraldsson
Copy link
Contributor Author

Since this was reopened and is being (re)considered, I'm not sure what the "14 failing" are (false alarms?):

fatal: Couldn't find remote ref refs/pull/41638/merge

My code in the PR seems trivial enough that I think it's correct, so maybe it can just be merged for 1.8? If people regret it could be undone in e.g. 1.9 or even in 1.8.x.

@oscardssmith
Copy link
Member

we're past the point of merging this into 1.8. I think the main thing this needs is a rebase on the latest master.

src/jloptions.c Outdated Show resolved Hide resolved
@JeffBezanson JeffBezanson added the needs news A NEWS entry is required for this change label Mar 31, 2022
src/jloptions.c Outdated Show resolved Hide resolved
@JeffBezanson
Copy link
Member

From triage: OK. See comments. Also need to update the manual.

@oscardssmith
Copy link
Member

(and a rebase)

@PallHaraldsson
Copy link
Contributor Author

PallHaraldsson commented Mar 31, 2022

a) "(and a rebase)", I sort of know it means outside of GitHub, and even though GitHub also has a page on it, it seems impossible here in the web-only (where I made the edit/PR...):

https://docs.github.com/en/get-started/using-git/about-git-rebase

When asked to do a rebase previously, I want down a rabbit-hole, made some mistakes, and had to start over. Can anyone else do it for me on this PR number?

Outdated (not sure why it went away...): b) The code seems fine and the timeout(s) seem like false alarms:

git cat-file -e 5ddb1708b1800937898888c655db83ada0727d1d
 in dir /buildworker/worker/package_linuxaarch64/build (timeout 1200 secs)
..
git fetch -f -t https://github.com/JuliaLang/julia refs/pull/41638/merge --progress
 in dir /buildworker/worker/package_linuxaarch64/build (timeout 1200 secs)
fatal: Couldn't find remote ref refs/pull/41638/merge

@oscardssmith oscardssmith removed the triage This should be discussed on a triage call label Apr 14, 2022
@oscardssmith oscardssmith merged commit 641be31 into JuliaLang:master Apr 14, 2022
@PallHaraldsson PallHaraldsson deleted the patch-2 branch April 15, 2022 17:40
@JeffBezanson JeffBezanson removed the needs news A NEWS entry is required for this change label Nov 10, 2022
sjkelly added a commit to sjkelly/julia that referenced this pull request Mar 22, 2024
The behavior was changed and the CLI doc was removed
in JuliaLang#41638, though we current still allow users to
selectively use the `@fastmath` macro.
This adds back the CLI docs and makes a minor
clarification about behavior.
sjkelly added a commit to sjkelly/julia that referenced this pull request Mar 22, 2024
The behavior was changed and the CLI doc was removed
in JuliaLang#41638, though we current still allow users to
selectively use the `@fastmath` macro.
This adds back the CLI docs and makes a minor
clarification about behavior.
sjkelly added a commit to sjkelly/julia that referenced this pull request Mar 25, 2024
The behavior was changed and the CLI doc was removed
in JuliaLang#41638, though we current still allow users to
selectively use the `@fastmath` macro.
This adds back the CLI docs and makes a minor
clarification about behavior.

Co-authored-by: Matt Bauman <mbauman@gmail.com>
IanButterworth pushed a commit to sjkelly/julia that referenced this pull request Mar 27, 2024
The behavior was changed and the CLI doc was removed
in JuliaLang#41638, though we current still allow users to
selectively use the `@fastmath` macro.
This adds back the CLI docs and makes a minor
clarification about behavior.

Co-authored-by: Matt Bauman <mbauman@gmail.com>
mbauman added a commit that referenced this pull request Mar 27, 2024
The behavior was changed and the CLI doc was removed
in #41638, though we current still allow users to
selectively use the `@fastmath` macro.
This adds back the CLI docs and makes a minor
clarification about behavior.

Co-authored-by: Matt Bauman <mbauman@juliahub.com>
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.

deprecate global --math-mode=fast flag?
4 participants