android / ndk

The Android Native Development Kit
2.02k stars 259 forks source link

Unified headers: basename in string.h defined for all versions if defined(__cplusplus) #440

Closed fornwall closed 6 years ago

fornwall commented 7 years ago

From <string.h> in NDK r15b:

#if defined(__USE_GNU) && !defined(basename)
/*
 * glibc has a basename in <string.h> that's different to the POSIX one in <libgen.h>.
 * It doesn't modify its argument, and in C++ it's const-correct.
 */
#if defined(__cplusplus)
extern "C++" char* basename(char* _Nonnull) __RENAME(__gnu_basename);
extern "C++" const char* basename(const char* _Nonnull) __RENAME(__gnu_basename);
#else

#if __ANDROID_API__ >= 23
char* basename(const char* _Nonnull) __RENAME(__gnu_basename) __INTRODUCED_IN(23);
#endif /* __ANDROID_API__ >= 23 */

#endif
#endif

Is it really correct that basename is declared if C++ is used regardless of __ANDROID_API__ while android-23 is required for the C function?

fornwall commented 7 years ago

Likewise with strchrnul:

#if defined(__USE_GNU)
#if defined(__cplusplus)
extern "C++" char* strchrnul(char* _Nonnull, int) __RENAME(strchrnul) __attribute_pure__;
extern "C++" const char* strchrnul(const char* _Nonnull, int) __RENAME(strchrnul) __attribute_pure__;
#else

#if __ANDROID_API__ >= 24
char* strchrnul(const char* _Nonnull, int) __attribute_pure__ __INTRODUCED_IN(24);
#endif /* __ANDROID_API__ >= 24 */

#endif
#endif
jmgao commented 7 years ago

Oops, this slipped through because the tool we have to verify correctness only builds the headers in C.

Header fix at https://android-review.googlesource.com/428060, but the preprocessor needs to be updated to build with C++ as well to insert guards.

DanAlbert commented 7 years ago

The user facing part of this is fixed, so taking off r16. @jmgao still needs to fix the versioner to make sure we don't have more mistakes like this one, but not necessary for r16 (aiui we're not going to be adding more like these anyway since libc++ already provides const-correct overloads for most of the C library).

enh commented 7 years ago

did we introduce new problems for strcasestr and memrchr with https://android-review.googlesource.com/#/c/platform/bionic/+/455204/?

DanAlbert commented 7 years ago

Guessing @jmgao won't be able to do any work on the versioner any time soon.

enh commented 7 years ago

answering my own question about strcasestr and memrchr: no, they're fine because they're already surrounded by __ANDROID_API__ checks for other reasons, or were there from the beginning anyway.

jmgao commented 6 years ago

AFAIK, the versioner support needed for this has been done for a while, should this be closed?

jmgao commented 6 years ago

(After scrolling up and rereading the comments, yeah, looks like it.)