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

Recognize that compareUnsigned is safe on Byte and Short. #64

Merged
merged 1 commit into from
Jul 23, 2024

Conversation

cpovirk
Copy link
Contributor

@cpovirk cpovirk commented Jul 22, 2024

They have been always desugared since
https://r8.googlesource.com/r8/+/4b8c252dbd34d0b8d1d5defe7bbe5da288eed677,
which came shortly after support for Long and Integer,
https://r8.googlesource.com/r8/+/62ef970fea988efa2bbfc81e68d1072d5ab930d7,
which is already recognized by Gummy Bears.

They have been always desugared since
https://r8.googlesource.com/r8/+/4b8c252dbd34d0b8d1d5defe7bbe5da288eed677,
which came shortly after support for `Long` and `Integer`,
https://r8.googlesource.com/r8/+/62ef970fea988efa2bbfc81e68d1072d5ab930d7,
which is already recognized by Gummy Bears.
@cpovirk
Copy link
Contributor Author

cpovirk commented Jul 22, 2024

This should perhaps be more of a feature request than a PR, as I'm not at all confident that I've edited the right place :) But I figured I'd attempt it as a PR in case it does turn out to be this easy.

@cpovirk
Copy link
Contributor Author

cpovirk commented Jul 22, 2024

Ha, and as soon as I pushed this, I looked at my latest testing results, and I realized that the actual problem was a failure in our JVM Animal Sniffer check, as this API doesn't exist until Java 9 (and we check for compatibility with Java 8)!

It's entirely possible that Gummy Bears already recognizes this under Android; my testing just didn't actually get that far, and I misread the error message.

I'll test some more and then let you know what I find.

@cpovirk
Copy link
Contributor Author

cpovirk commented Jul 22, 2024

OK, my mistake on the JVM side aside, I do still think that these APIs are always desugared, and I now have some evidence that this PR makes them be recognized as such:

Before:

[INFO] --- animal-sniffer:1.23:check (check-java-version-compatibility) @ guava ---
[INFO] Checking unresolved references to com.toasttab.android:gummy-bears-api-21:0.8.0
[ERROR] /usr/local/google/home/cpovirk/tmp.49wlVCLfVo/git/android/guava/src/com/google/common/primitives/UnsignedBytes.java:127: Undefined reference: int Byte.compareUnsigned(byte, byte)

After:

[INFO] --- animal-sniffer:1.23:check (check-java-version-compatibility) @ guava ---
[INFO] Checking unresolved references to com.toasttab.android:gummy-bears-api-21:0.8.1-SNAPSHOT

Now, in our particular use case, it's probably actually safer for us not to tell Animal Sniffer that, since we want our Android code to also work under a Java 8 JVM :) But I think this PR makes sense for testing actual Android compatibility. Feel free to merge or close it, as you prefer.

@ogolberg
Copy link
Member

Thanks

@ogolberg ogolberg merged commit 2448b8c into open-toast:main Jul 23, 2024
1 check passed
@cpovirk cpovirk deleted the byteshort branch July 23, 2024 17:03
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