Re: Decimal: Formal review begins.

From: Ruben Perez (rubenperez038_at_[hidden])
Date: 2025-01-17 21:44:59

On Fri, 17 Jan 2025 at 22:16, Christopher Kormanyos <e_float_at_[hidden]> wrote:
>
> >>> BOOST_DECIMAL_DISABLE_CLIB ifdefs-out
> >>> entire headers - wouldn't it be simpler
> >>> to have a subset of headers allowable
> >>> in embedded systems, with others just
> >>> labelled as "not supported"?
>
> Detailed comments follow below, but
> I personally doubt if there would
> be anything remotely simple about having
> different amounts of headers. To the contrary,
> it would add more complexity.
>
> It has been a while since we invented these
> compiler switches and I actually needed to
> review them myself.
>
> BOOST_DECIMAL_DISABLE_CLIB seems to be
> going in the direction (if you follow this)
> of the freestanding movement in C++26.
> Basically, BOOST_DECIMAL_DISABLE_CLIB
> disables all potentially heavyweight
> components. I can list what these are
> in a second post. So you might not
> get <string> or <charconv>-like support
> on the metal.

No, I haven't been following. What follows are my thoughts, and
upfront apologies if I got something wrong.

>From what I've seen, there's a __STDC_HOSTED__ macro that tells you
whether you're in bare metal (value 0) or not (value 1). Then, some
headers have some parts of them ifdef-ed out when the macro is set to
1 (freestanding mode).

Peeking at libcstdc++ (gcc-13) implementation, which supports this,
there are headers that do this (when there's some definitions that are
present on freestanding implementations and others that are not), and
headers that require a hosted implementation to be included. For
instance, fstream, which is not supported in freestanding mode, does
the following:

#include <bits/requires_hosted.h> // iostreams

// Rest of the contents, unguarded

This file will issue an error in freestanding mode:

#include <bits/c++config.h>

#if !_GLIBCXX_HOSTED // this is defined to __STDC_HOSTED__
# error "This header is not available in freestanding mode."
#endif

#endif

All the headers in decimal I've seen would fall in the second
category: their contents are either fully available in freestanding
mode, or not at all.

So I think it would make sense to follow what libstdc++ does.

In any case (and apologies if I got this wrong), it looks like the
approach you have stems from only supporting the convenience header,
and not the other smaller headers. I'd say that something like:

// Freestanding OK
#include <boost/decimal/decimal32.hpp>
#include <boost/decimal/decimal32_fast.hpp>
// ...
#ifndef BOOST_DECIMAL_DISABLE_CLIB
#include <boost/decimal/cstdlib.hpp>
#include <boost/decimal/detail/io.hpp>
// ...
#endif

Would make sense. Of course, this requires you to slightly rework some
parts of the implementation.

Regards,
Ruben.

>
> BOOST_DECIMAL_DISABLE_CLIB has the secondary
> effect of *also* defining
> BOOST_DECIMAL_DISABLE_IOSTREAM (see below).
> Disabling I/O streaming only cuts out
> headers like <iostream>, <sstream> and
> their buddies.
>
>
> >> The functions are only used in tests
> >> because they are for the end user.
> >> We have no need for streaming in the
> >> implementation. Since this is a
> >> header-only library I am not worried
> >> about library incompatibilities from
> >> different configurations.
>
> > I think we might be talking about different
> > things here. Grepping for
> > BOOST_DECIMAL_DISABLE_IOSTREAM,
> > it protects the following functions:
> >  * debug_pattern: not documented and
> >    excluded from coverage
> >  * bit_string: not documented
> >  * Streaming native/emulated,
> >    signed/unsigned 128/256 integer types,
> >    all of which are in namespace detail.
>
> BOOST_DECIMAL_DISABLE_IOSTREAM does pretty
> much *literally what it says. It disables
> I/O streaming functions.
>
> In contrast to BOOST_DECIMAL_DISABLE_CLIB,
> which disables a lot more like <string>
> and the like and a bunch of stuff that's
> not already by its nature more or less
> freestanding.
>
> To be honest, I am completely satisfied
> with the inner workings of these compiler
> options at the moment. We don't have
> freestanding yet, so this situation
> sort of mocks that up.
>
>
>
>
> On Friday, January 17, 2025 at 08:36:06 PM GMT+1, Ruben Perez via Boost <boost_at_[hidden]> wrote:
>
>
> >
> > > > I never tried many of the permutations of headers outside of the convenience one. The library is structured to match the STL so that it is unsurprising to the average user. I think you could pick and choose if you wanted to.
> > >
> >
> > >
> >
> > > I tried picking charconv and it didn't work :)
> > >
> >
> > > I think that the actual header structure is good, matching STL as you
> > > said. I don't agree with recommending users to always include the
> > > entire library - I think that increases compile times without much
> > > benefit. Hence I was asking whether there was an actual reason to do
> > > it.
> > >
> >
> >
> > So there's things I have to manually set that most never worry about for builtin types like global rounding mode and float evaluation method. Some impl headers need forward declarations of the types, some don't. It's pretty convenient from a design perspective to make things just work with no effort on the part of the user. I'm sure everything could be made to work piecemeal, but for a difference of maybe 3 seconds of compile time it's not worth the effort.
>
> I'm afraid I don't agree here. Since this is a header-only library,
> this is 3 seconds added to both direct and indirect users of the
> library. If all Boost libraries did this, compile times would become
> unmanageable. Also, Boost doesn't have the best reputation in this
> aspect, so I think taking care of this is valuable.
>
> Having a set of public headers that work is established practice in
> Boost, and I'd advise to follow it.
>
> >
> > > > > 3. In the line of the previous question, is there a reason to have
> > > > > BOOST_DECIMAL_DISABLE_IOSTREAM instead of splitting iostream
> > > > > functionality to a separate header? In my experience, the more config
> > > > > macros you have, the more chances of getting bugs. Also, is the test
> > > > > suite being run with these macros defined?
> > > >
> >
> > > > We have a the options to disable a bunch of the clib functionality so that the library can run on embedded platforms. We do have QEMU of an STM board in the CI which tests all of this. Why test embedded you ask? It's not uncommon for finance devs to run on bare metal platforms.
> > >
> >
> > >
> >
> > > I understand the objective, and I think it's great having tests for
> > > that. But I don't think the method is the best.
> > >
> >
> > > I've reviewed all uses of BOOST_DECIMAL_DISABLE_IOSTREAM, and if I'm
> > > reading this correctly, they all guard functions that are exclusively
> > > used in the tests. I don't think these functions should be in the
> > > headers shipped to users, but in the tests.
> > >
> >
> > > I acknowledge that these functions require access to private members
> > > of public classes, so I guess that's why they are defined there. I use
> > > a dummy friend struct placed in the detail namespace when I have such
> > > problems (I think I copied the pattern from Boost.Json). I think you
> > > can get rid of all the iostream includes altogether doing this (except
> > > for the ones in io.hpp, which are actually not guarded by the macro).
> > >
> >
> > > BOOST_DECIMAL_DISABLE_CLIB ifdefs-out entire headers - wouldn't it be
> > > simpler to have a subset of headers allowable in embedded systems,
> > > with others just labelled as "not supported"?
> >
> > The functions are only used in tests because they are for the end user. We have no need for streaming in the implementation. Since this is a header-only library I am not worried about library incompatibilities from different configurations.
>
> I think we might be talking about different things here. Grepping for
> BOOST_DECIMAL_DISABLE_IOSTREAM, it protects the following functions:
>
> * debug_pattern: not documented and excluded from coverage
> * bit_string: not documented
> * Streaming native/emulated, signed/unsigned 128/256 integer types,
> all of which are in namespace detail.
>
> Is the end user expected to use any of these?
>
> >
> > Matt
>
> Regards,
> Ruben.
>
>
> _______________________________________________
> Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost