Re: Decimal: Formal review begins.

From: Matt Borland (matt_at_[hidden])
Date: 2025-01-16 16:50:26

> I didn't make my point here clear, sorry. I completely agree with
> including every cstdio function, except for sprintf. There's no way to
> communicate sprintf the length of the input buffer, so it's very easy
> to end up with buffer overflows. I can't think of a use case where
> sprintf should be used over snprintf, which just adds the length
> parameter. So that's the only function I was advising to be removed.

That makes sense to me. I think all the major compilers issue warnings on sprintf usage already.

> 

> > > Some more questions:
> > > 1. As a user, when should I pick decimal64 vs decimal64_fast? I intend
> > > to implement support for your decimal types in the static interface of
> > > Boost.MySQL as part of this review - should I support decimalXY,
> > > decimalXY_fast, or both?
> > 

> > It would be a pretty easy template to support decimalXY and decimalXY_fast. I know for fact decimalXY is being used out in industry, but not sure about decimalXY_fast. It would be more space efficient to store as decimalXY.
> 

> 

> I will implement both. It's true that from my library's perspective it
> doesn't matter. But I think as a user, I'd like to see some guidelines
> on what use cases should I employ each one for.
> 

A guidelines section would be easy enough to add. Since I expect Peter Turcan to provide a laundry list of solid feedback on the docs we will hold for now.

> > > 2. Is there a rationale behind only supporting the convenience header,
> > > as stated in the docs? Including the entire library in my machine
> > > (gcc-12, Ubuntu 22.04, -std=c++23) is about 4.5seconds - similar to
> > > the entire Asio in magnitude. Including only decimal32.hpp cuts the
> > > time to around 1s.
> > 

> > 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.

> > > 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.

Matt