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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from
Apr 14, 2022
Merged

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

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 •
edited
Loading

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 PallHaraldsson mentioned this pull request Aug 6, 2021
Closed
@PallHaraldsson
Copy link
Contributor Author

PallHaraldsson commented Aug 12, 2021 •
edited
Loading

@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 •
edited
Loading

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.

JeffBezanson
JeffBezanson reviewed Mar 31, 2022
View reviewed changes
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
JeffBezanson
JeffBezanson reviewed Mar 31, 2022
View reviewed changes
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 •
edited
Loading

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
@oscardssmith oscardssmith mentioned this pull request Apr 14, 2022
Merged
@PallHaraldsson PallHaraldsson deleted the patch-2 branch April 15, 2022 17:40
@ViralBShah ViralBShah mentioned this pull request Sep 6, 2022
Closed
@PallHaraldsson PallHaraldsson mentioned this pull request Sep 6, 2022
Closed
@c42f c42f mentioned this pull request Sep 30, 2022
Closed
@JeffBezanson JeffBezanson removed the needs news A NEWS entry is required for this change label Nov 10, 2022
This was referenced May 17, 2023
Open
Open
@Zentrik Zentrik mentioned this pull request Aug 11, 2023
Closed
sjkelly added a commit to sjkelly/julia that referenced this pull request Mar 22, 2024
@sjkelly
Harmonize and add docs for --math-mode
0f4b0bb 
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 sjkelly mentioned this pull request Mar 22, 2024
Merged
sjkelly added a commit to sjkelly/julia that referenced this pull request Mar 22, 2024
@sjkelly
Harmonize and add docs for --math-mode
bbfd62d 
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
@sjkelly @mbauman
Harmonize and add docs for --math-mode
e8ac6e7 
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
@sjkelly @mbauman @IanButterworth
Harmonize and add docs for --math-mode
cb50c67 
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
@sjkelly @mbauman
Harmonize and add docs for --math-mode (#53818)
7a62dff 
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
Reviewers

@oscardssmith oscardssmith oscardssmith left review comments

@JeffBezanson JeffBezanson JeffBezanson left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

deprecate global --math-mode=fast flag?
4 participants
@PallHaraldsson @KristofferC @oscardssmith @JeffBezanson