Skip to content

lib: cpp: Introduce FULL_LIBCPP_SUPPORTED similar to C version #57445

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 3, 2023

Conversation

galak
Copy link
Collaborator

@galak galak commented May 1, 2023

Introduce Kconfig symbol FULL_LIBCPP_SUPPORTED that is similar to the C version FULL_LIBC_SUPPORTED. This way we can utilize the same pattern in tests and samples to filter for when a full libc++ is needed.

@galak
Copy link
Collaborator Author

galak commented May 1, 2023

plan to use this for thrift samples to start.

@cfriedt
Copy link
Member

cfriedt commented May 1, 2023

plan to use this for thrift samples to start.

In true FB fashion, there is yet another thrift that has been open-sourced.

https://github.com/facebook/fbthrift

This one seems like it would be significantly less resource intensive because it uses std::coroutine and std::string_view.

I would love to put together an ISO C code generator on the other hand.. Apache Thrift relies on glib unfortunately

@keith-packard
Copy link
Collaborator

Would be good to see this in actual use to make sure it works as advertised.

Copy link
Member

@stephanosio stephanosio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are adding FULL_LIBCPP_SUPPORTED, shall we add MINIMAL_LIBCPP_SUPPORTED, GLIBCXX_LIBCPP_SUPPORTED and others as well?

@galak
Copy link
Collaborator Author

galak commented May 1, 2023

Since we are adding FULL_LIBCPP_SUPPORTED, shall we add MINIMAL_LIBCPP_SUPPORTED, GLIBCXX_LIBCPP_SUPPORTED and others as well?

I figured we would add them if they where needed.

@galak
Copy link
Collaborator Author

galak commented May 1, 2023

Would be good to see this in actual use to make sure it works as advertised.

Here's how this would be used:

#57097

@galak
Copy link
Collaborator Author

galak commented May 1, 2023

@stephanosio do you think we should add select FULL_LIBCPP_SUPPORTED for EXTERNAL_LIBCPP? Should we also limit EXTERNAL_LIBCPP to NATIVE_APPLICATION. Since it seems to exist purely for native_posix from what I can tell.

cfriedt
cfriedt previously approved these changes May 2, 2023
@stephanosio
Copy link
Member

do you think we should add select FULL_LIBCPP_SUPPORTED for EXTERNAL_LIBCPP?

No, because EXTERNAL_LIBCPP in itself does not imply a full standard C++ library (e.g. one could provide their own standard C++ library, similar to the Zephyr minimal C++ library, that implements a limited subset of the standard C++ library).

Should we also limit EXTERNAL_LIBCPP to NATIVE_APPLICATION. Since it seems to exist purely for native_posix from what I can tell.

No, because this option is intended to be selected when an external standard C++ library is used (e.g. when user/application supplies their own standard C++ library).

So, I would say we should leave the selection of the FULL_LIBCPP_SUPPORTED up to whoever/whatever is supplying that LIBCPP.

In case of the NATIVE_APPLICATION, it should select FULL_LIBCPP_SUPPORTED based on the aforementioned reasoning; in fact, it already selects FULL_LIBC_SUPPORTED:

zephyr/Kconfig.zephyr

Lines 317 to 319 in 58088f3

config NATIVE_APPLICATION
bool "Build as a native host application"
select FULL_LIBC_SUPPORTED

stephanosio
stephanosio previously approved these changes May 2, 2023
Introduce Kconfig symbol FULL_LIBCPP_SUPPORTED that is similar to the
C version FULL_LIBC_SUPPORTED.  This way we can utilize the same
pattern in tests and samples to filter for when a full libc++ is
needed.

Signed-off-by: Kumar Gala <kumar.gala@intel.com>
@carlescufi carlescufi merged commit e268f8e into zephyrproject-rtos:main May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants