Skip to content

-Xlint:captured to warn on *Ref boxing#8691

Closed
hrhino wants to merge 1 commit intoscala:2.12.xfrom
hrhino:topic/xlint-captured
Closed

-Xlint:captured to warn on *Ref boxing#8691
hrhino wants to merge 1 commit intoscala:2.12.xfrom
hrhino:topic/xlint-captured

Conversation

@hrhino
Copy link
Contributor

@hrhino hrhino commented Feb 3, 2020

Maybe we need an -Xslow instead, for stuff that does what you expect but maybe not quite as fast. It's a testament either to the power of scalac's warning capability or of the simplicity of this feature that it's implemented in only around 5 lines of code.

Maybe we need an -Xslow instead, for stuff that does what you expect but
maybe not quite as fast. It's a testament either to the power of
scalac's warning capability or of the simplicity of this feature that
it's implemented in only around 5 lines of code.
@@ -0,0 +1 @@
-Xfatal-warnings -Xlint:captured No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be // scalac: -Werrror etc in the test source.

Wait, you know this, what year is this?

Is this that thing where you PR against an old branch and you never know what features are supported?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the thing where I'm on my phone hotspot and Seth just upgraded sbt so I can't switch to 2.13.x, and I didn't really want to go poking around in git to see what the latest 2.13.x commit I can build on is, and moreover I do know this because I keep telling myself I'll backport it to the scala-partest repo and bribe a kind Lightbender to release a new partest, or I could just not bother.

I get this from you every time, btw.

Do we even want this in 2.12? @mkeskells might, or @diesalbla.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-Werrror should be the variant that deletes the source file because it emitted a warning.

@retronym
Copy link
Member

retronym commented Feb 4, 2020

This seems like a somewhat drastic lint warning IMO. Maybe its nice to have it available, but I feel like this would make plenty of folks using -Xlint:_ search for the way to write -Xlint:_,-captured or otherwise do busywork to heed the advice which isn't targetted with a profiler.

If you have a benchmark that simulates the application usage, its pretty easy to find the worthwhile places to avoid such captures by looking for the allocation stack traces of the xRef classes.

@som-snytt
Copy link
Contributor

Is it almost on a par with -Xlint:nonlocal-return?

Can anyone enable -Xlint without exceptions?

Not all -Wunused are enabled by lint, so maybe -Wperformance?

@dwijnand
Copy link
Member

dwijnand commented Feb 4, 2020

Maybe -Xlint / -Xlint:_ should have some groupings, where this lives in the extended/paranoid/all-the-things category. See GHC (https://downloads.haskell.org/~ghc/8.8.2/docs/html/users_guide/using-warnings.html) which has default on warnings, -W warnings, and (unfortunately named) -Wall and -Weverything. Rust also has some groups: https://doc.rust-lang.org/rustc/lints/groups.html.

@hrhino hrhino closed this Feb 4, 2020
@hrhino hrhino reopened this Feb 4, 2020
@hrhino
Copy link
Contributor Author

hrhino commented Feb 4, 2020

I think this has hit a -Wall.

@hrhino hrhino closed this Feb 4, 2020
@SethTisue
Copy link
Member

agree this more properly would be part of a separate linter or Scalafix ruleset

@som-snytt
Copy link
Contributor

As hrhino pointed out, it's one LOC here.

It was Ichoran who pointed out once how pernicious it is when you rewrite your while loop and accidentally ruin performance. I don't really want to look at stack traces while I'm refactoring.

I agree it's maybe noisy for -Xlint. I guess the jury is still out on -Xlint:recurse-with-default.

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.

6 participants