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

Add atomicfu plugin and use it to implement all atomics #963

Merged
merged 11 commits into from
Jan 2, 2024

Conversation

m-sasha
Copy link
Member

@m-sasha m-sasha commented Dec 21, 2023

Proposed Changes

As atomicfu atomics are now stable, we can use them to implement AtomicReference, AtomicInt etc.

I made the changes module-by-module, so it could be more convenient to review commit-by-commit.

Breaking change

This will break code in the specific source-sets that was using the public actual.

The list of types that were public and are now internal:

  • androidx.compose.runtime.AtomicReference in JS, native and WASM source-sets.
  • androidx.compose.foundation.AtomicLong in JS, WASM and native source-sets.
  • androidx.compose.material.InternalAtomicReference in JS and native source-sets.

Testing

Test: Waiting to see the tests in CI.

Copy link

github-actions bot commented Dec 21, 2023

Test Results

66 tests  ±0   66 ✅ ±0   0s ⏱️ ±0s
10 suites ±0    0 💤 ±0 
10 files   ±0    0 ❌ ±0 

Results for commit 7738b42. ± Comparison against base commit ba6c6d4.

♻️ This comment has been updated with latest results.

@m-sasha m-sasha force-pushed the m-sasha/update-atomicfu branch 2 times, most recently from 9b1cc64 to fe2ac4e Compare December 21, 2023 18:37
@m-sasha m-sasha requested a review from igordmn December 22, 2023 08:48
@m-sasha m-sasha force-pushed the m-sasha/update-atomicfu branch from fe2ac4e to 7738b42 Compare December 27, 2023 23:21
compose/animation/animation-core/build.gradle Outdated Show resolved Hide resolved

actual class AtomicLong actual constructor(value: Long) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's add ## Breaking change to the PR description. It will be added manually to the changelog later (or via some future script)

Copy link
Collaborator

Choose a reason for hiding this comment

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

actual was a (public) class (not a typealias) and it was using the declaration

We should write it from the user perspective. The user sees only available classes, not that there are aliases/actuals. We should enumarate all removed classes and sourceSets (jsMain, iosMain, wasmMain).

@igordmn igordmn requested a review from eymar January 2, 2024 10:56
@igordmn
Copy link
Collaborator

igordmn commented Jan 2, 2024

@eymar, could you also look at the PR?

import kotlinx.atomicfu.atomic

internal actual class AtomicReference<V> actual constructor(value: V) {
Copy link
Member

Choose a reason for hiding this comment

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

I see the source set here named jbMain. I understand that we added jbMain in runtime module, because skikoMain name doesn't fit runtime purpose and it doesn't depend on skiko.

Since animation-core depends on ui, then it depends on skiko:
implementation(project(":compose:ui:ui"))

And the name can be skikoMain to not introduce a new name beside runtime. Not a big thing though. No skiko code is used here directly, so skikoMain name here is arguable.

jvmMain.dependsOn(jbMain)
jsMain.dependsOn(jbMain)
jsNativeMain.dependsOn(jbMain)
nativeMain.dependsOn(jbMain)
Copy link
Member

Choose a reason for hiding this comment

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

if suggestion by Igor above to be applied, then here it's similar:

desktopdMain.dependsOn(jbMain)
jsNativeMain.dependsOn(jbMain)

@eymar
Copy link
Member

eymar commented Jan 2, 2024

Intereseting. Wasn't https://android-review.googlesource.com/c/platform/frameworks/support/+/2875994 a breaking change? I see no api files changes in it.

@igordmn
Copy link
Collaborator

igordmn commented Jan 2, 2024

Wasn't https://android-review.googlesource.com/c/platform/frameworks/support/+/2875994 a breaking change?

Only for native/js. For desktop/Android it was internal. It was available in commonMain, but it wouldn't compile, because the actual was internal. Discussion

@m-sasha m-sasha force-pushed the m-sasha/update-atomicfu branch from 7738b42 to 9fc3e71 Compare January 2, 2024 15:09
@m-sasha m-sasha merged commit f805adb into jb-main Jan 2, 2024
6 checks passed
@m-sasha m-sasha deleted the m-sasha/update-atomicfu branch January 2, 2024 19:26
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.

3 participants