This is an archive of the discontinued LLVM Phabricator instance.

Don't expose unavailable cstdio functions.
ClosedPublic

Authored by danalbert on Mar 12 2020, 12:26 PM.

Details

Reviewers
EricWF
mclow.lists
ldionne
Group Reviewers
Restricted Project
Commits
rGff87813715ec: Don't expose unavailable cstdio functions.
Summary

These aren't available on Android in all configurations.

Diff Detail

Event Timeline

danalbert created this revision.Mar 12 2020, 12:26 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a subscriber: krytarowski. · View Herald Transcript
ldionne requested changes to this revision.Mar 12 2020, 3:52 PM
ldionne added a subscriber: ldionne.
ldionne added inline comments.
libcxx/include/__config
1550

I believe this would be easier to understand if it were instead _LIBCPP_HAS_NO_FGETPOS. The condition would become:

#if defined(__ANDROID__) && defined(__USE_FILE_OFFSET64) && __ANDROID_API < 24

Do you agree?

This revision now requires changes to proceed.Mar 12 2020, 3:52 PM
danalbert updated this revision to Diff 250101.Mar 12 2020, 4:53 PM
danalbert marked an inline comment as done.
danalbert added inline comments.
libcxx/include/__config
1550

Yep, SGTM. Also corrected __ANDROID__ to __BIONIC__, not that there are any non-android bionic users old enough (or 32-bit enough) for this to be a problem.

ldionne requested changes to this revision.Mar 13 2020, 10:33 AM

Are there no required changes to the tests as well? I see libcxx/test/std/depr/depr.c.headers/stdio_h.pass.cpp using these declarations.

This revision now requires changes to proceed.Mar 13 2020, 10:33 AM

Yep, good point. I think we don't run the full libc++ test suite for the broken configuration. Fixed.

Could we add a .fail.cpp test that ensures when _LIBCPP_HAS_NO_FGETPOS is defined we don't actually have it?

Something like:

using T = decltype(::fgetpos); 
#ifdef _LIBCPP_HAS_NO_FGETPOS
// expected-error@-2 {{no such thing}}
#else
// expected-no-diagnostics
#endif
libcxx/include/__config
1550

Do we really need two macros for this? I would prefer one.

danalbert updated this revision to Diff 250331.Mar 13 2020, 5:44 PM
danalbert marked 2 inline comments as done.

Add .fail test, merge into single macro.

danalbert updated this revision to Diff 250332.Mar 13 2020, 5:53 PM

(remove some trailing whitespace that snuck in)

danalbert updated this revision to Diff 250334.Mar 13 2020, 5:55 PM
Harbormaster completed remote builds in B49199: Diff 250334.
ldionne requested changes to this revision.Mar 20 2020, 8:37 AM
ldionne added inline comments.
libcxx/test/std/depr/depr.c.headers/no_fgetpos_fsetpos.fail.cpp
2

Missing license comment and also // REQUIRES: verify-support.

And I believe this test should be in libcxx/test/libcxx, not in` libcxx/test/std`, cause it is libc++ specific.

This revision now requires changes to proceed.Mar 20 2020, 8:37 AM
danalbert updated this revision to Diff 255806.Apr 7 2020, 2:14 PM

Added missing copyright header and moved into the libcxx specific test directory.

danalbert marked an inline comment as done.Apr 7 2020, 2:14 PM
EricWF accepted this revision.Apr 7 2020, 2:52 PM
ldionne accepted this revision.Apr 7 2020, 3:01 PM
This revision is now accepted and ready to land.Apr 7 2020, 3:01 PM
This revision was automatically updated to reflect the committed changes.