Skip to content

Try to make code android 6 (SDK 23) compatible #4237

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

Closed
wants to merge 3 commits into from

Conversation

shanjunmei
Copy link

Its implementation is very simple and we can foresee that it will not cause any side effects

@iProdigy
Copy link
Contributor

the try-catch approach is likely overkill (see original fix commit 0ede935#diff-e3b0b5408a33e269ed50f7c3a5c222d9c66715351d0ac5bbde1719a5ee50253f)

unfortunately this merge reversed that patch (see file at prior commit)

@shanjunmei
Copy link
Author

shanjunmei commented Nov 30, 2023

unfortunately this merge reversed that patch (see file at prior commit)

before i review jdk source code ,i have same concern ,

public Class<?>[] getParameterTypes() { return parameterTypes.clone(); } public int getParameterCount() { return parameterTypes.length; } this is copy from jdk source code

@shanjunmei
Copy link
Author

unfortunately this merge reversed that patch (see file at prior commit)

before i review jdk source code ,i have same concern ,

public Class<?>[] getParameterTypes() { return parameterTypes.clone(); } public int getParameterCount() { return parameterTypes.length; } this is copy from jdk source code

We can find that if we directly replace it with the length of parameterTypes, it will clone parameterTypes .

@iProdigy
Copy link
Contributor

another note: getParameterCount is also used elsewhere, so this patch alone won't allow lowering the min android sdk version (which is currently 26, and even 2.13 didn't support android 6)

@shanjunmei
Copy link
Author

another note: getParameterCount is also used elsewhere, so this patch alone won't allow lowering the min android sdk version (which is currently 26, and even 2.13 didn't support android 6)

For me, this one modification is enough. Of course, if necessary, I will be happy to make similar fixes to all similar codes.

@iProdigy
Copy link
Contributor

Of course, if necessary, I will be happy to make similar fixes to all similar codes.

i don't think the maintainers are interested in that approach: #3702 (comment)

you'll likely have to use a forked version of jackson in your app if you want sdk 23 compatibility

@shanjunmei
Copy link
Author

Of course, if necessary, I will be happy to make similar fixes to all similar codes.

i don't think the maintainers are interested in that approach: #3702 (comment)

you'll likely have to use a forked version of jackson in your app if you want sdk 23 compatibility

In fact, I have already done this, but I don’t want to miss the new updates of Jaskson in the future. I believe that the original worry was also due to the performance loss caused by clone, but now this problem will no longer exist. hi @cowtowncoder ,can you help check this approach

@shanjunmei
Copy link
Author

I understand that we need to keep pace with the times, but sometimes we also have to adapt and understand the reality. Just like Android, it lags behind Java’s feature support, and even the official has come up with a solution to desugar.

@shanjunmei
Copy link
Author

another note: getParameterCount is also used elsewhere, so this patch alone won't allow lowering the min android sdk version (which is currently 26, and even 2.13 didn't support android 6)

btw, I have done the same for all getParameterCount calls based on 2.1.7

@cowtowncoder
Copy link
Member

No, not going go back -- we have gone down this path multiple times.

We do have Animal Sniffer check for Android SDK compatibility and baseline we have (26 I think) and absolutely no plans to try to reduce it.

So this will not be accepted.

@cowtowncoder cowtowncoder changed the title android 6 compatible Try to make code android 6 (SDK 23) compatible Nov 30, 2023
@shanjunmei
Copy link
Author

No, not going go back -- we have gone down this path multiple times.

We do have Animal Sniffer check for Android SDK compatibility and baseline we have (26 I think) and absolutely no plans to try to reduce it.

So this will not be accepted.

As you said, this situation has happened many times. Doesn’t this mean that there is something wrong with the current approach?

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