potswa / cxx_function

Prototype for new std::function features, compatible with C++11.
MIT License
46 stars 7 forks source link

Base class of allocator_mismatch_error #17

Open FrankHB opened 5 years ago

FrankHB commented 5 years ago

https://github.com/potswa/cxx_function/blob/master/cxx_function.hpp#L627

Is there any reason that the exception is derived directly from std::exception, rather than a more specific type like std::invalid_argument?

potswa commented 5 years ago

I think that this was considered already. Git blame might provide some insight. (I’ll be on mobile for a while now. Can’t find a blame here.)

Deriving from a more specific class adds a dependency on <stdexcept>. What’s the benefit? I don’t know of any cases where it’s helpful to generalize over logic_errors, or how logic_error is more practically specific than exception when it cannot be specifically handled.

FrankHB commented 5 years ago

I think that this was considered already. Git blame might provide some insight. (I’ll be on mobile for a while now. Can’t find a blame here.)

I don't find the rationale. It is not in P0043R0 either. (BTW, what is the status of the paper?) I don't ever find documentation for the interface itself.

Deriving from a more specific class adds a dependency on . What’s the benefit? I don’t know of any cases where it’s helpful to generalize over logic_errors, or how logic_error is more practically specific than exception when it cannot be specifically handled.

The concerns are mainly about logical correctness. Different to exception classes that directly inherit std::exception (like std::bad_function_call), I assume allocator_mismatch_error will be only throw for relatively specific conditions and it should not be expected in arbitrary contexts (as I don't write catch(std::exception&) everywhere for some unknown exceptions). A general exception base class would be somewhat confusing for the intented use. I see the class std::logical_error is designed exactly for the purpose, and std::invalid_argument further constraints the use cases.

Once the exception class is published, making it more-derived is likely to be an ABI-breaking change (cf. std::ios_base::failure), because the layout is not guaranteed compatible. So unless the assumption is convincingly false, it is better done sooner than later.

A minor benefit is allowing different exception messages determined by the client code without a new exception class or breaking ABI, although this is not quite often needed in practice and some implementations have extensions on std::exception.

As of the dependency on <stdexcept>, I don't see it quite relevant. This facility of diagnostics seems essentially not orthogonal to interface like <functional>, but more fundamental. And for implementations with exceptions already enabled, the binary overhead should not be significant, or even nearly to zero (e.g. MSVC has already the complexicity in its std::exception implementation). Anyway, if the differences are not negligible, why not a narrow contract instead of a new exception type?

potswa commented 5 years ago

I assume allocator_mismatch_error will be only throw for relatively specific conditions

The key word is "relatively." It's thrown when an object with a specific allocation policy (a function_container) receives a target object value created using a different type of allocator.

So if function_container is only used in specific contexts, then those should be guarded.

But if you're guarding those contexts, then isn't catch(allocator_mismatch_error) just what you want? I still don't understand how catch(logic_error) or catch(invalid_argument) is an improvement over catch(exception).

As far as I know, invalid_argument is mostly used as a proxy to UNIX EINVAL. It tends to reflect a complaint that some raw input data failed validation, or a generic reference such as a filesystem handle refers to a resource of the wrong type. But this error doesn't concern the type or value; it concerns resource allocation.

Once the exception class is published, making it more-derived is likely to be an ABI-breaking change

But so is breaking derivation: Either way, standardized is standardized. New extensions to the standard don't seem to be making this design decision consistently. I would welcome and follow any clear guidance from the committee, but for now I am influenced by:

  1. A vague recollection of surveying some C++ expert colleagues when writing it,
  2. Personal bias toward a minimalist approach in the absence of a use-case or example,
  3. Binary compatibility for this library, regardless of what might be standardized.
FrankHB commented 5 years ago

It's thrown when an object with a specific allocation policy (a function_container) receives a target object value created using a different type of allocator.

I see it for the cases concerned here. Nevertheless, given with the name, it can be more generic, not always with function_container. I wonder the guideline about where and when to throw such an exception in general, if appropriate.

But if you're guarding those contexts, then isn't catch(allocator_mismatch_error) just what you want?

This is the most direct way. However, throwing a new exception should not always require having a new path to catch in every piece of client code - basically it needs to modify many instances when the throwing code is in a library and it is buried in a long dependency chain among various client projects. It is about how to minimize the maintenance work on the existing code when some function is replaced by function_container or something else.

Surely there can exist a smooth route: wrapping the code in the library, which can throw the new exception, and then using the wrapped pieces of code to replace old code not throwing the new exception for each clients one by one. But if the base exception is already expected by the clients, such wrappers might be not needed at all. Using std::exception is certainly an overkill (at least I as long as I don't want to catch std::bad_alloc), so it should be avoided.

That said, I don't think catch(logic_error) or catch(invalid_argument) is sufficiently good. The work is not eliminated automatically. But there do exist improvement for some cases, compared to catch(exception) or just adding catch(allocator_mismatch_error) anywhere.

As far as I know, invalid_argument is mostly used as a proxy to UNIX EINVAL. It tends to reflect a complaint that some raw input data failed validation, or a generic reference such as a filesystem handle refers to a resource of the wrong type. But this error doesn't concern the type or value; it concerns resource allocation.

Not exactly. It used to be true before we have std::system_error. Now it should get closed to the original meaning - just a general error for invalid input for some parameters (but occasionally heavily used in UNIX-flavor low-level interface designs), as I don't see std::invalid_argument endorsed to be a better choice to std::system_error(std::errc::invalid_argument) , which is different to preferring std::bad_alloc over std::system_error(std::errc::not_enough_memory) stated by [syserr.syserr.overview]/2.

The exceptional cases naturally include the failure on verification of function parameters in both low-level and high-level code. Actually, at least one constructor of std::bitset can throw std::invalid_argument for invalid argument values, so there is already the prior art.

Either way, standardized is standardized. New extensions to the standard don't seem to be making this design decision consistently.

I'm OK to stay with the status quo of the library design. However, since this library is a referential source of some proposals to be standardized, the question left here is yet to be resolved. Hope we can have some general guidelines on this topic before making it into the standard.

potswa commented 5 years ago

It's probably worth mentioning that the committee direction around allocators is toward dynamic polymorphism, which avoids the need for this exception.

Of course, my intent was that this exception should be suitable for all classes combining allocator awareness with type erasure.

Of all the proposals prototyped by this library, the allocation part is (at least for now) the most dead.