This is an archive of the discontinued LLVM Phabricator instance.

libc++: move checks for newlib to actually work
ClosedPublic

Authored by LittleFox94 on May 13 2020, 10:34 AM.

Details

Reviewers
EricWF
ldionne
Group Reviewers
Restricted Project
Commits
rG8deaa4a1471d: [libc++] Move checks for newlib to actually work
Summary

The checks did not work in __config, since no header defining _NEWLIB_VERSION was included before. This patch moves the two checks for newlib to the headers that actually need it - and after they already include relevant headers.

Diff Detail

Event Timeline

LittleFox94 created this revision.May 13 2020, 10:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 13 2020, 10:34 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Since this include was deleted as "dead code" before, I added a comment why it is not dead

EricWF requested changes to this revision.May 13 2020, 10:57 AM
EricWF added a subscriber: EricWF.
EricWF added inline comments.
libcxx/include/__config
13

Please do this later in the file,

Specifically after we #define __has_include in the case where we don't have it.

This revision now requires changes to proceed.May 13 2020, 10:57 AM

I am slightly concerned that this also introduces a bunch of declarations in __config, which is the most included file of the library. That would also be the first time we include unistd.h in the libc++ headers (as opposed to the sources used to build the dylib). That might not be an issue, but it's something to keep in mind.

Is this necessary for libc++ to work with Newlib? I thought we already supported it as-is (we certainly have some code paths that are conditionals to it).

I am slightly concerned that this also introduces a bunch of declarations in __config, which is the most included file of the library. That would also be the first time we include unistd.h in the libc++ headers (as opposed to the sources used to build the dylib). That might not be an issue, but it's something to keep in mind.

Another way would be to include _newlib_version.h if it exists, would that be better? Would then be a newlib specific patch.

Is this necessary for libc++ to work with Newlib? I thought we already supported it as-is (we certainly have some code paths that are conditionals to it).

__config has a check for newlib which never works, due to the preprocessor macro not being set without an include (line 933 in the patched revision)

We could lift the two macros that depend on _NEWLIB_VERSION into the headers that use them.
Then we'll avoid the include in config.

We could lift the two macros that depend on _NEWLIB_VERSION into the headers that use them.
Then we'll avoid the include in config.

sounds good to me. If this is also good for @ldionne I'll prepare a new patch :)

We could lift the two macros that depend on _NEWLIB_VERSION into the headers that use them.
Then we'll avoid the include in config.

sounds good to me. If this is also good for @ldionne I'll prepare a new patch :)

Yeah, I'd like to see that patch.

Is this review still relevant? If not, let's abandon it to clear up the review queue.

Is this review still relevant? If not, let's abandon it to clear up the review queue.

Ouch oops.. sorry, was really scatterbrained this year .. I'll look into it in the next 2 days, feel free to close afterwards if you don't read from me

Again, sorry

LittleFox94 retitled this revision from libc++: include unistd.h if available to get some preprocessor macros to libc++: move checks for newlib to actually work.
LittleFox94 edited the summary of this revision. (Show Details)
ldionne requested changes to this revision.Apr 8 2021, 1:05 PM

LGTM with nitpick. Once updated, do you need someone to commit this for you? If so, please provide Author Name <email@domain> for attribution. Thanks!

libcxx/include/fstream
201

You can remove this level of #if, we never expect users to define it, and I'd rather not give the impression that we allow users to.

This revision now requires changes to proceed.Apr 8 2021, 1:05 PM

Removed an unnecessary check as requested in review

LittleFox94 marked an inline comment as done.Apr 11 2021, 4:24 PM

Removed the unnecessary check as requested.

I cannot commit myself, please use "Mara Sophie Grosch <littlefox@lf-net.org>" as commit author. Thank you and sorry it took so long.

ldionne accepted this revision.Apr 12 2021, 11:19 AM
This revision was not accepted when it landed; it landed in state Needs Review.Apr 12 2021, 11:21 AM
This revision was automatically updated to reflect the committed changes.