Skip to content
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

Split TopLevelExportDefs away from LinkedClass #4124

Merged
merged 2 commits into from Jul 16, 2020

Conversation

gzm0
Copy link
Contributor

@gzm0 gzm0 commented Jul 15, 2020

Side-effects for TopLevelMethodExportDefs in IRChecker:

  • Allow them on all class kinds.
  • Allow names containing __ for non JS classes.
  • Disallow references to JS class captures (this was a bug).

@gzm0 gzm0 requested a review from sjrd July 15, 2020 06:54
@gzm0
Copy link
Contributor Author

gzm0 commented Jul 15, 2020

Split away from #4111. To be merged now, or not :)

Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Let's merge this now.

@@ -42,8 +43,6 @@ object Infos {
val methods: List[MethodInfo],
val jsNativeMembers: Map[MethodName, JSNativeLoadSpec],
val exportedMembers: List[ReachabilityInfo],
Copy link
Member

Choose a reason for hiding this comment

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

The CI says:

Suggested change
val exportedMembers: List[ReachabilityInfo],
val exportedMembers: List[ReachabilityInfo]

def loadClassDefAndVersion(irFile: IRFileImpl)(
implicit ec: ExecutionContext): Future[(ClassDef, Option[String])] = {
update(irFile).map(s => (s._1, version))
}

private def update(irFile: IRFileImpl)(
implicit ec: ExecutionContext): Future[(ClassDef, Infos.ClassInfo)] = synchronized {
implicit ec: ExecutionContext): Future[(ClassDef, Infos.ClassInfo, List[Infos.TopLevelExportInfo])] = synchronized {
Copy link
Member

Choose a reason for hiding this comment

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

The CI says that this line is too long (more than 120 characters). Perhaps move the synchronized inside?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took the chance to simplify this a bit. PTAL

gzm0 added 2 commits July 16, 2020 06:48
This will help with organizing modules.

Side-effects for TopLevelMethodExportDefs in IRChecker:
- Allow them on all class kinds.
- Allow names containing `__` for non JS classes.
- Disallow references to JS class captures (this was a bug).
@gzm0 gzm0 merged commit 430f915 into scala-js:master Jul 16, 2020
@gzm0 gzm0 deleted the split-top-level-exports branch July 16, 2020 08:46
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.

None yet

2 participants