Page MenuHomePhabricator

Rework libcxx strerror_r handling.
ClosedPublic

Authored by jyknight on Jun 16 2017, 2:22 PM.

Details

Summary

The set of #ifdefs used to handle the two incompatible variants of
strerror_r were not complete (they didn't handle newlib appropriately).

Rather than attempting to make the ifdefs more complex, make them
unnecessary by choosing which behavior to use dependent upon the
return type.

Diff Detail

Repository
rL LLVM

Event Timeline

jyknight created this revision.Jun 16 2017, 2:22 PM

New one is harder to comprehend and less portable (usage of __atribute__).

Can we just replace

#elif defined(__linux__) && !defined(_LIBCPP_HAS_MUSL_LIBC) &&                 \
    (!defined(__ANDROID__) || __ANDROID_API__ >= 23)

with #elif __GLIBC__?

Or better:

#elif __GLIBC__ || (__ANDROID_API__ - 0) >= 23

If I understand correctly, Android adopted GNU version.

New one is harder to comprehend and less portable (usage of __atribute__).

I can't disagree more strongly. This is fundamentally portable C++ code -- attribute((unused)) is simply warning suppression.
I used it directly, because I saw other .cpp files in libcxx already did so. If it needs to be conditioned, it could be.

Or better:

#elif GLIBC || (ANDROID_API - 0) >= 23
If I understand correctly, Android adopted GNU version.

No, we can't use that conditional, because android and glibc aren't the only ones to have adopted the GNU variant. As I mentioned, newlib uses it also.

We *may* be able to use a different conditional than what's there or what you suggested, but solving the problem once and for all by using the function type is certainly a better solution.

New one is harder to comprehend and less portable (usage of __atribute__).

I can't disagree more strongly. This is fundamentally portable C++ code -- attribute((unused)) is simply warning suppression.
I used it directly, because I saw other .cpp files in libcxx already did so. If it needs to be conditioned, it could be.

OK with me.

EricWF, wdyt?

waltl accepted this revision.Jun 26 2017, 1:36 PM
This revision is now accepted and ready to land.Jun 26 2017, 1:36 PM
This revision was automatically updated to reflect the committed changes.