-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
KE2: Extract lambda expressions #18110
Conversation
28022ce
to
f9fd733
Compare
fun getLocallyVisibleFunctionLabelMapping( | ||
key: KaFunctionSymbol | ||
): LocallyVisibleFunctionLabels { | ||
lock.withLock { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will only be used in a TrapWriter for a source file, not a diagnostic TRAP writer, so it isn't shared and won't need to use a lock. I think I'll take a look at refactoring TrapLabelManager
to make it easier to use locks only when necessary, but for now let's at least add a TODO comment reminding us to come back to this.
key: KaFunctionSymbol, | ||
add: (KaFunctionSymbol) -> LocallyVisibleFunctionLabels | ||
): LocallyVisibleFunctionLabels { | ||
lock.withLock { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto TODO comment
|
||
is KtLambdaExpression -> { | ||
// Propagate extraction to the wrapped function literal: | ||
return extractExpression(e.functionLiteral, callable, parent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are all function literals wrapped by a lambda expression?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so.
* return invoke(args[0] as T0, args[1] as T1, ..., args[I] as TI) | ||
* } | ||
* } | ||
* ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Useful comments, thanks!
java/kotlin-extractor2/src/main/kotlin/entities/FunctionalInterface.kt
Outdated
Show resolved
Hide resolved
elementToReportOn: PsiElement, | ||
compilerGeneratedKindOverride: CompilerGeneratedKinds? = null, | ||
superConstructorSelector: (KaFunctionSymbol) -> Boolean = { it.valueParameters.isEmpty() }, | ||
extractSuperConstructorArgs: (Label<DbSuperconstructorinvocationstmt>) -> Unit = {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just inline the default here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can now be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we might need to put this back later to cover this case: https://github.com/github/codeql/blob/main/java/kotlin-extractor/src/main/kotlin/KotlinFileExtractor.kt#L7942-L7961
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry, I missed that. Anyway, like you say, we can always add it back if we need it.
java/kotlin-extractor2/src/main/kotlin/entities/FunctionalInterface.kt
Outdated
Show resolved
Hide resolved
23c66fc
to
7d50eb5
Compare
No description provided.