[PATCH] D86596: Implement __isOSVersionAtLeast for Android

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added inline comments.


================
Comment at: compiler-rt/lib/builtins/os_version_check.c:232-234
+  if (__system_property_get("ro.build.version.sdk", buf) == 0) {
+    // When the system property doesn't exist, defaults to future API level.
+    SdkVersion = __ANDROID_API_FUTURE__;
----------------
jiyong wrote:
> smeenai wrote:
> > This is inconsistent with `android_get_device_api_level`, which returns -1 on failure instead (per https://developer.android.com/ndk/reference/group/apilevels#android_get_device_api_level). How come a different behavior was chosen here? I don't know if it's at all likely for reading this property to fail anyway, but if it does, it seems confusing for an availability check to go in the opposite direction as a manual check based on `android_get_device_api_level`.
> > How come a different behavior was chosen here?
> 
> The reasoning behind this code is that the exceptional case has to be resolved by this runtime library (in isOSVersionAtLeast), not by application, and isOSVersionAtLeast has no means of reporting an error; it returns boolean. So, when ro.build.version.sdk doesn't exist isOSVersionAtLeast can't help but return one of these three:
> 
> (1) return false regardless of Major
> 
> (2) return Major >=  a_reasonably_old_api_level
> 
> (3) return true regardless of Major
> 
> We thought (1) is bad because it renders all APIs as unavailable, which probably doesn't reflect the reality. (2) is not good because people have different opinions on what a_reasonably_old_api_level should be. So we picked (3) as the least worst option.
> 
> The situation is different when users use android_get_device_api_level manually. If it returns -1, they can fallback to check the availability of the symbol manually by calling dlsym or similar. 
> 
> ----------------
> Nevertheless, practically speaking, ro.build.version.sdk is guaranteed to exist. So this behavior won't be triggered anyway.
> 
That makes sense, thanks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86596/new/

https://reviews.llvm.org/D86596