Skip to content

Conversation

@smarter
Copy link
Member

@smarter smarter commented Apr 21, 2015

Review by @DarkDimius

@smarter smarter force-pushed the simplify/makeBridgeDef branch 4 times, most recently from 284b54b to fbd4626 Compare April 21, 2015 20:02
@DarkDimius
Copy link
Contributor

Am I missing something or changes are:

  • one less error reporting(that instead will fail in non-obvious place in typeAssinger);
  • renaming of variables(newDefSym -> implDefSym, parentSym -> overridenSym);
  • previous method was able to run before erasure(and be reused by specialization), this one cannot as assumes single parameter list?

@smarter
Copy link
Member Author

smarter commented Apr 21, 2015

  • The error method was not used, that's why I removed it.
  • Yep, I also added comments
  • Ah, I didn't know that being able to reuse it for specialization was planned, if you prefer I can drop this part of the changes.

@DarkDimius
Copy link
Contributor

The error method was not used, that's why I removed it.

It was used, line 531.

Yep, I also added comments

Comment looks fine for me.

I'd prefer to maintain previous names, as I dont find new ones much better, and this commit touches most lines without good reason, making following git history of changes harder. (I frequently use git annotate to see why the code is written in some particular way)

@smarter smarter force-pushed the simplify/makeBridgeDef branch from fbd4626 to 0ee9f23 Compare April 26, 2015 22:38
@smarter smarter changed the title Erasure#Typer#makeBridgeDef: simplify implementation Erasure#Typer: Document makeBridgeDef Apr 26, 2015
@smarter
Copy link
Member Author

smarter commented Apr 26, 2015

OK, commit changed to only add documentation to makeBridgeDef, I still think the renaming was useful though (you can always call git annotate/blame on an old revision).

@DarkDimius
Copy link
Contributor

LGTM

DarkDimius added a commit that referenced this pull request Apr 27, 2015
Erasure#Typer: Document makeBridgeDef
@DarkDimius DarkDimius merged commit 9ba09d4 into scala:master Apr 27, 2015
tgodzik added a commit to tgodzik/scala3 that referenced this pull request Jul 23, 2025
Backport "Add regression test for issue 22585" to 3.3 LTS
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.

2 participants