Skip to content

Erasure#Typer: Document makeBridgeDef #491

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

Merged
merged 1 commit into from
Apr 27, 2015

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
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