Skip to content
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

empty_init convenience #63

Closed
breese opened this issue Sep 28, 2019 · 19 comments · Fixed by #65
Closed

empty_init convenience #63

breese opened this issue Sep 28, 2019 · 19 comments · Fixed by #65

Comments

@breese
Copy link
Contributor

breese commented Sep 28, 2019

Instantiating boost::empty_value with arguments is done using the empty_init_t tag. This means that we have to write something like base(empty_init_t{}, args....

Can we add an empty_init instance of that tag, so we can write base(empty_init, args...)?

We can avoid the usual problems of header-only instantiations of a global object by changing struct empty_init_t {}; into enum empty_init_t { empty_init };.

@glenfe
Copy link
Member

glenfe commented Oct 1, 2019

A BOOST_STATIC_CONSTEXPR empty_init_t empty_init = empty_init_t(); should satisfy that request too, right?

@breese
Copy link
Contributor Author

breese commented Oct 2, 2019

That will work.

The reason I mention the enum alternative, is that when I had to investigate the binary size of a large embedded project, I found a boost::none (from Boost.Optional) instance in each translation unit that used this constant. While this is not a major problem per se, the enum solution completely avoids it, which is why I prefer enum for tagging.

@Lastique
Copy link
Member

Lastique commented Oct 2, 2019

I suppose, we should mark boost::none inline on C++17 compilers. That should mitigate binary size bloat.

@glenfe
Copy link
Member

glenfe commented Oct 2, 2019

It's also why I never created an empty_init constant initially. The four extra characters "_t()" did not seem like.a big deal. :)

Users will always create constants if they want them, maybe even shorter names than the ones we might provide.

(We could even just create a type alias for empty_init_t to a shorter type name.)

@Lastique
Copy link
Member

Lastique commented Oct 2, 2019

I think the mental pattern to assign a global value (think nullopt, nullptr, nothrow) is too strong. And asking users to create constants isn't solving anything - they will still have the binary bloat. It only shifts the burden on the users.

@breese
Copy link
Contributor Author

breese commented Oct 2, 2019

There is an issue for boost::none.

C++17 inline constant is good general solution, but for integers or tags the enum solution works with older compilers as well.

@Lastique
Copy link
Member

Lastique commented Oct 2, 2019

Enum, if we're talking about unscoped enums, has the potential to be converted to int, which can bite if a function overload accepts int and a type constructible from the enum. I believe, the compiler should pick the int overload because this is a builtin conversion as opposed to a user-defined constructor.

@pdimov
Copy link
Member

pdimov commented Oct 2, 2019

constexpr is automatically inline, but none isn't BOOST_STATIC_CONSTEXPR.

@pdimov
Copy link
Member

pdimov commented Oct 2, 2019

Wait, no, this was only true for static class members. Namespace-scope constexpr variables aren't automatically inline. Sad.

Maybe we need to add inline to BOOST_STATIC_CONSTEXPR.

@glenfe
Copy link
Member

glenfe commented Oct 2, 2019

I'm not a fan of the enum approach; I prefer keeping empty_init_t a mono state type (like nullopt_t, nothrow_t, inplace_t). Maybe a new Boost.Config macro, with this intention in mind. e.g. BOOST_INLINE_CONSTEXPR which is inline constexpr if available, static constexpr if not.

@Lastique
Copy link
Member

Lastique commented Oct 2, 2019

Honestly, I don't see the point of BOOST_STATIC_CONSTEXPR (or BOOST_INLINE_CONSTEXPR) given that we already have BOOST_CONSTEXPR_OR_CONST. It might be useful to add BOOST_INLINE_VARIABLE, which expands to inline or nothing depending on BOOST_NO_CXX17_INLINE_VARIABLES.

@glenfe
Copy link
Member

glenfe commented Oct 2, 2019

Sounds fine to me. So in this case, usage would be of the form: BOOST_INLINE_VARIABLE BOOST_CONSTEXPR_OR_CONST type name = type(); ?

@Lastique
Copy link
Member

Lastique commented Oct 2, 2019

Yes.

@glenfe
Copy link
Member

glenfe commented Oct 2, 2019

And in the case where BOOST_NO_CXX17_INLINE_VARIABLES is defined, we're fine with the above too? i.e. We don't want that to have static or be in an unnamed namespace?

@Lastique
Copy link
Member

Lastique commented Oct 2, 2019

Yes, we're fine. Namespace-scope constants have internal linkage by default.

@glenfe
Copy link
Member

glenfe commented Oct 2, 2019

@pdimov Do you remember why bind couldn't have the placeholder constants in GCC versions before 4?

#if defined(__BORLANDC__) || defined(__GNUC__) && (__GNUC__ < 4)

@pdimov
Copy link
Member

pdimov commented Oct 2, 2019

BOOST_INLINE_VARIABLE BOOST_CONSTEXPR_OR_CONST is missing the current static though. Although I'm not sure whether it's needed. Not sure why this is any improvement over BOOST_INLINE_CONSTEXPR that expands to whatever's correct.

@pdimov
Copy link
Member

pdimov commented Oct 2, 2019

No, I don't.

@pdimov
Copy link
Member

pdimov commented Oct 2, 2019

Looking at the history, it was probably a precompiled header problem.

Lastique added a commit to Lastique/core that referenced this issue Oct 29, 2019
glenfe pushed a commit that referenced this issue Nov 24, 2019
* Added a convenience instance of empty_init_t.

Closes #63.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants