Re: [boost] [system] Would it be possible to trial a breaking change to Boost.System and see what happens?

Subject: Re: [boost] [system] Would it be possible to trial a breaking change to Boost.System and see what happens?
From: Niall Douglas (s_sourceforge_at_[hidden])
Date: 2018-01-12 23:58:57

>> 1. Stop treating code zero as always meaning success irrespective of
>> category. This remedies a defect where some custom error code domains
>> cannot be represented by error_code due to using value 0 for an error.
>> In actual code, I've never seen anyone ever use comparison to anything
>> but a default constructed error code, so I think this will be safe.
> 
> In my code I'm using contextual conversion to bool extensively, e.g. "if
> (err)" or "if (!err)". This is currently equivalent to comparing against
> 0, so I assume it will be broken by this change. So, you can count me
> right now as the one (loudly) complaining, among many others, I think.

Sorry, I forgot to mention that the category would be asked whether the
error code's current value is success or failure (its's Friday, and my
current contract is many hours drive from home. Sorry)

This is unavoidable because system_category (on POSIX and Win32) and
generic_category both define value = 0 as success. In some other error
code domain, -1 might mean success. Or 9999. Up to the domain. No
requirement that 0 be set aside as special, as currently.

>> 2. Success becomes solely the default constructed error code, which is
>> code zero and a new category of "null_category". This is internally
>> represented by { 0, nullptr }, and thus makes the default constructor
>> trivial which is highly desirable as it eliminates any compiler magic
>> static fencing for the default constructor. Right now the default
>> constructor uses system_category with value 0, and I suspect no code
>> will notice this change either.
> 
> This breaks "category()" as it returns a reference now.

It would still return a reference, just as now. You would check for
internal nullptr, if so return a static null_category.

> Also, "message()" has to check for a null category now.

I don't think that much cost relative to constructing a std::string.

>> 3, Make the default constructor constexpr, now possible. No code should
>> notice a difference, except less runtime code will be generated.
> 
> Not sure there will be any difference in the generated code, since the
> constructor still has to initialize the int and the pointer. Optimizers
> are already able to propagate constants even if the code is not constexpr.

But it cannot elide the potential call to an extern function, the
function which retrieves the error_category&. And therefore must emit
code, just in case the system_category might be fetched.

The present design of error_code generates code bloat when used by
Outcome or Expected. Changing it as I describe stops forcing the
compiler to emit code, and thus allows the compiler to fold more code
during optimisation, thus emitting less assembler. Tighter, less bloaty
code is the result.

> The real benefit, it seems, is the ability to use error_code in constant
> expressions. I'm not sure how useful this is, I've never had a case
> where I needed this.

This is not a motivator for me at least. My issue is the side effects on
code bloat. It annoys me when the fix is within easy reach.

>> 4. error_code::message() returns a std::string_view instead of
>> std::string if running C++ 17 or better. This lets us remove the
>> <string> dependency, and thus stop dragging in half the STL with
>> <system_error> which in turn makes <system_error> actually useful to the
>> embedded systems crowd. The guess here is that std::string_view has such
>> excellent interoperation with std::string that I think 99% of code will
>> compile and work perfectly without anybody noticing a thing.
> 
> This will only work if "error_category::message()" returns a string from
> a static storage. It will not allow relying on strerror_r or similar
> API, for example. This will make migration problematic if users define
> their own error categories that rely on external API like that.

You are correct that the no 4 change is by far the most consequential
proposed. I do not believe it affects end user code except for those who
inherit from error_code and override message() - surely a very, very
tiny number of people - but it sure as hell breaks everybody who
implements a custom error code category if you change the category's
message() signature.

The number of people worldwide currently implementing their own error
categories is likely low, and me personally speaking feel that their
mild inconvenience is worth losing <string> from the header
dependencies. But if backwards compatibility were felt to be super
important, in error_code::message() you could try dynamic_cast to an
error_category2 first in order to utilise an
error_category2::message_view() function. If the dynamic_cast fails, you
statically cache the strings returned by error_category::message() and
return string_views of them.

One could also, of course, simply drop modification no 4 entirely.
Modifications 1 to 3 are likely safe to almost all existing C++. Only
modification 4 breaks anything.

>> Boost was originally intended as an incubator for C++ standard changes.
>> This ticks that box perfectly.
>>
>> Thoughts on feasibility?
> 
> Even besides that my code is likely to be broken by this, I think this
> is a really bad idea. Sure, Boost was concieved as the playground for
> the features that are potentially later included into the standard, but
> it is also a major C++ library that is widely used in the industry.
> Breaking users' code willy-nilly does not do any good neither to Boost
> nor to its users. So no, no breaking changes to see how bad its smells
> afterwards, please.

This is an API breaking change being discussed. Code _might_ break
beyond custom error categories.

But I currently believe that code other than custom error categories
_won't_ break. We are changing the API in ways that I would be very,
very surprised if much real world code out there even notices.

Besides, there is precedent for standard library features in C++ 11
which came from Boost continuing to be evolved in Boost, whilst
retaining API compatibility, with said evolutions being mirrored by the
C++ standard later. All code which compiles now will still compile with
this modified Boost.System apart from custom error categories, this is
not in doubt. The question is whether all code will still function
exactly as before. This is what the trial would discover.

Finally, Boost has broken API much more severely than this in the past
as part of natural evolution (not always intentionally). If it's
preannounced that we're doing this, and we do it, I don't see a problem
here. Plenty of old APIs have been retired before. And besides, this is
for testing a proposed change to a future C++ standard. That's a great
reason to break API, if you're going to do it.

Niall

-- 
ned Productions Limited Consulting
http://www.nedproductions.biz/ http://ie.linkedin.com/in/nialldouglas/