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
Use subcolon args to simplify optimizer options #9810
Conversation
ba7d4f0
to
23de4bd
Compare
5ca6c01
to
dc9b785
Compare
obviously, allowing |
dc9b785
to
2a97faa
Compare
I thought I wasn't supposed to update the build, but I must have tossed a bad coin.
Edit:
Edit: undeprecate the old options because no point in 2.13. Also, some usages of |
6f8fe94
to
ab29500
Compare
Thank you for taking this on! I see two issues with the current state:
Could we make |
I will defer to your expert opinion on those two gotchas. The only use case for fine-grained options is, "The optimizer broke my code, so I have to turn off a feature for this file." I agree, it was a nuisance to fiddle with On not defaulting to double star, it would be nice if there were a convenient default, such as, inline from classes with sources in this build. Then an option to inline from dependencies; from std lib; from platform lib. But maybe real programmers don't mind saying I will follow up with the tighter screws before I forget how it works. |
About defaulting to I think requiring people to think about inlining when enabling it is a good thing :) |
Thanks lrytz, you have me singing the classic tune, "Puttin' on the Rytz". Probably I've made that pun before. The new behavior is much better. One difference is that before, you needed Specifying only one option would silently not inline anything. Now, |
fbbe635
to
b36f386
Compare
I needed this the other day when I couldn't remember how to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you very much! A few minor suggestions, if you agree I'm happy to push them, what ever works best.
|
||
val inlineFrom = Choice( | ||
"inline", | ||
s"Inline method invocations from specified sites; enables all optimizations; see -Yopt-inline-heuristics.\n$inlineHelp", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s"Inline method invocations from specified sites; enables all optimizations; see -Yopt-inline-heuristics.\n$inlineHelp", | |
s"Inline method invocations, enables all optimizations. Requires specifying sites from where to inline, see below. See also -Yopt-inline-heuristics.\n\n$inlineHelp", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason this is unresolved is that I missed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I pushed approximately that. I had already added a newline to the inlineHelp
. It also follows -opt-inline-from:help
.
Grouping options `l:none`, `l:default`, `l:method`, `l:inline`, are renamed `none`, `default`, `local`, `all`. `-opt-inline-from:**` is dropped in favor of `-opt:inline:**`. `-opt-warnings:_` is renamed `-Wopt:_`. The ambiguity in parsing `-option:command:a,b,c`, where `-Wconf` sees values `command:a`, `b`, `c`, is resolving by special- casing `MultiChoiceSetting` only.
b36f386
to
05218d2
Compare
05218d2
to
655d3e1
Compare
@som-snytt do you want a second round of review from Lukas, or are you reasonably confident you've addressed his concerns and we should go ahead and merge? |
yes, now it is quite gorgeous after following up his tweaks. It is tweakèd beauty. |
The old settings ( |
Optimizer is enabled by
-opt:inline:**
, as described in the new overview. Warnings are enabled by-Wopt
and verbose output by-Vinline
.Fixes scala/bug#11873