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

SI-10020 SI-10027 Scaladoc: keep Java comment scanning stack-friendly [ci: last-only] #5469

Merged
merged 4 commits into from Nov 4, 2016

Conversation

Projects
None yet
6 participants
@adriaanm
Member

adriaanm commented Oct 20, 2016

... and boy scout the area a bit, reducing duplication.

I was led to this part of the forest while investigating some stack overflows during Java comment scanning. The regression (in #5246, /cc @jodersky FYI) was a result of some hooks into formerly tail-recursive methods. They are once more stack-friendly, and also finally final.

Functionality is intended to be identical.

Review by @som-snytt because boy scout?

adriaanm added some commits Oct 19, 2016

Keep `skipBlockComment` tail recursive
Avoid StackOverflow on big comments.
Simplify `ScaladocJavaUnitScanner` while in there.

TODO: Do same for `ScaladocUnitScanner`?
DocScanner has doc-comment scanning hooks.
Align the Scala and Java doc comment scanning methods a bit.
The Scala one especially had gotten a bit messy,
with regular block comments being kind of accumulated,
but never actually registered as DocComments.

@scala-jenkins scala-jenkins added this to the 2.12.1 milestone Oct 20, 2016

@adriaanm adriaanm changed the title from Keep comment scanning stack-friendly to Keep comment scanning stack-friendly [ci: last-only] Oct 20, 2016

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Oct 20, 2016

Member

/rebuild I'm pretty sure there's little difference between these commits in terms of what the test suite could reveal.

Member

adriaanm commented Oct 20, 2016

/rebuild I'm pretty sure there's little difference between these commits in terms of what the test suite could reveal.

@SethTisue

This comment has been minimized.

Show comment
Hide comment
@SethTisue

SethTisue Oct 21, 2016

Member

/rebuild

Member

SethTisue commented Oct 21, 2016

/rebuild

@som-snytt

This comment has been minimized.

Show comment
Hide comment
@som-snytt

som-snytt Oct 21, 2016

Contributor

I'll take a look because "friendly", although the scouts won't take me back.

Contributor

som-snytt commented Oct 21, 2016

I'll take a look because "friendly", although the scouts won't take me back.

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Oct 21, 2016

Member

Last commit is green -- squash on merge?

Member

adriaanm commented Oct 21, 2016

Last commit is green -- squash on merge?

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Nov 3, 2016

Member

/nothingtoseehere -- test failures in earlier commits were due to jenkins load

Member

adriaanm commented Nov 3, 2016

/nothingtoseehere -- test failures in earlier commits were due to jenkins load

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Nov 3, 2016

Member

@jodersky, could I perhaps guilt you into a review? 😁

Member

adriaanm commented Nov 3, 2016

@jodersky, could I perhaps guilt you into a review? 😁

@jodersky

This comment has been minimized.

Show comment
Hide comment
@jodersky

jodersky Nov 3, 2016

Member

of course, such an embarrassing mistake 😳

Member

jodersky commented Nov 3, 2016

of course, such an embarrassing mistake 😳

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Nov 3, 2016

Member

Not at all! I didn't catch it either. OO bad. final good! Also, that method should've been explicitly @tailrec

Member

adriaanm commented Nov 3, 2016

Not at all! I didn't catch it either. OO bad. final good! Also, that method should've been explicitly @tailrec

@jodersky

This comment has been minimized.

Show comment
Hide comment
@jodersky

jodersky Nov 3, 2016

Member

LGTM! Maybe we should add a regression test like this 2.12.x...jodersky:SI-10027-test ? I can do it in a separate PR if you prefer to just get this fix in

Member

jodersky commented Nov 3, 2016

LGTM! Maybe we should add a regression test like this 2.12.x...jodersky:SI-10027-test ? I can do it in a separate PR if you prefer to just get this fix in

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Nov 3, 2016

Member

Thanks! Cherry picked.

Member

adriaanm commented Nov 3, 2016

Thanks! Cherry picked.

@adriaanm adriaanm changed the title from Keep comment scanning stack-friendly [ci: last-only] to SI-10020 SI-10027 Keep comment scanning stack-friendly [ci: last-only] Nov 3, 2016

/** To prevent doc comments attached to expressions from leaking out of scope
* onto the next documentable entity, they are discarded upon passing a right
* brace, bracket, or parenthesis.
*/

This comment has been minimized.

@som-snytt

som-snytt Nov 3, 2016

Contributor

I guess this could wait for scalafmt.

@som-snytt

som-snytt Nov 3, 2016

Contributor

I guess this could wait for scalafmt.

This comment has been minimized.

@adriaanm

adriaanm Nov 4, 2016

Member

Not my best spacing work.

@adriaanm

adriaanm Nov 4, 2016

Member

Not my best spacing work.

@adriaanm adriaanm merged commit 10c609e into scala:2.12.x Nov 4, 2016

5 checks passed

cla @adriaanm signed the Scala CLA. Thanks!
Details
integrate-ide [3452] SUCCESS. Took 8 s.
Details
validate-main [3983] SUCCESS. Took 53 min.
Details
validate-publish-core [3853] SUCCESS. Took 5 min.
Details
validate-test [3361] SUCCESS. Took 48 min.
Details
@som-snytt

This comment has been minimized.

Show comment
Hide comment
@som-snytt

som-snytt Nov 16, 2016

Contributor

Sorry I didn't get back to this. Looking now to see how to make a scaladoc test...

Contributor

som-snytt commented Nov 16, 2016

Sorry I didn't get back to this. Looking now to see how to make a scaladoc test...

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Nov 17, 2016

Member

No worries, my guilt-trip efforts yielded a test and a review.

Member

adriaanm commented Nov 17, 2016

No worries, my guilt-trip efforts yielded a test and a review.

@SethTisue SethTisue changed the title from SI-10020 SI-10027 Keep comment scanning stack-friendly [ci: last-only] to SI-10020 SI-10027 Scaladoc: keep Java comment scanning stack-friendly [ci: last-only] Dec 5, 2016

private def isDigit(c: Char) = java.lang.Character isDigit c
private var openComments = 0
protected def putCommentChar(): Unit = nextChar()
final protected def putCommentChar(): Unit = { processCommentChar(); nextChar() }

This comment has been minimized.

@kiritsuku

kiritsuku Dec 15, 2016

Contributor

Making this method final is a source incompatible change, which are supposed not to be done in minor Scala versions afaik. Concretely it broke the IDE. We can't update from 2.12.0 to 2.12.1. Since 2.12.1 is already released it is too late to revert the change. We may find a workaround for it but I wanted to mention in anyway.

@kiritsuku

kiritsuku Dec 15, 2016

Contributor

Making this method final is a source incompatible change, which are supposed not to be done in minor Scala versions afaik. Concretely it broke the IDE. We can't update from 2.12.0 to 2.12.1. Since 2.12.1 is already released it is too late to revert the change. We may find a workaround for it but I wanted to mention in anyway.

This comment has been minimized.

@SethTisue

SethTisue Dec 15, 2016

Member

We don't make those sorts of firm compatibility guarantees for compiler internals.

In the past, our shared backstop for this sort of thing was scripts/jobs/integrate/ide — if we broke something the IDE was using, it would tell us. But that script was never updated to work with 2.12, so it hasn't been part of PR validation or nightly testing for a long time now.

If we want to avoid this kind of thing happening, I think either that script would need to be restored to be part of Scala's CI again, or (to catch the problem a little later, but still relatively fast), you would have to add something comparable to your own CI that tests your code against 2.12.x (and 2.13.x?) nightlies.

@SethTisue

SethTisue Dec 15, 2016

Member

We don't make those sorts of firm compatibility guarantees for compiler internals.

In the past, our shared backstop for this sort of thing was scripts/jobs/integrate/ide — if we broke something the IDE was using, it would tell us. But that script was never updated to work with 2.12, so it hasn't been part of PR validation or nightly testing for a long time now.

If we want to avoid this kind of thing happening, I think either that script would need to be restored to be part of Scala's CI again, or (to catch the problem a little later, but still relatively fast), you would have to add something comparable to your own CI that tests your code against 2.12.x (and 2.13.x?) nightlies.

This comment has been minimized.

@kiritsuku

kiritsuku Dec 15, 2016

Contributor

Ok. I hope once we are fully compatible to 2.12.1 we can re-enable the IDE integration CI job.

@kiritsuku

kiritsuku Dec 15, 2016

Contributor

Ok. I hope once we are fully compatible to 2.12.1 we can re-enable the IDE integration CI job.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment