Strict Variant

Library Submission

Note: you must be a logged in registered user in order to submit a library!
  • logo_linkweb_linkcomment 
    Add a row
Display Statistics
Reviews There are 0 reviews
There are 5 comments

Comment on This Page

  • Robert Ramey says:

    This look pretty good to me. A few things I would like to see though:

    a) the usage of type requirements in the documentation and with static asserts or some other mechanism in the code to enforce the restrictions "no-throw destructible"
    b) the information in the git hub "readme" be included in the documentation as well – even if it means duplicating it.

    Good luck with this.

    • beckct says:

      Thanks for your comments. I overhauled the documentation and tried to make it more thorough / follow other boost libraries more closely. I think all the info from the README is now in the documentation. I already had some static asserts like you mentioned, but they weren't properly documented before.

      What other kinds of steps are appropriate for me to take? Should I solicit reviews on boost ML at some point, or is that not usually done?

      • Robert Ramey says:

        You've documented strict_variant::variant<T. …> as if it were a class. Why is that is a problem? Because this doesn't tell the user what kinds of types can be substituted for the T, ….
        For more information on this check out the "type requirements" section on the boost library incubator web site. There is also the video on "how to make a boost library" which addresses this subject.

        • beckct says:

          I see, I've watched your video now.

          So, let me make sure that I understand exactly what you are suggesting that I do.

          Currently the way I enforce these constraints is using `static_assert`'s here: https://github.com/cbeck88/strict-variant/blob/174852c68e54dae7647e7b1165186492fa27fc3d/include/strict_variant/variant.hpp#L79

          First thing, in the body of the class template. I thought that is a good way to enforce type constraints like this, since they will always be checked when the type is instantiated?

          They aren't shown in the Synopsis, but in the documentation I have a `Notes:` section which mentions the restrictions, and mentions that they are enforced by `static_assert`.

          Are you suggesting that:

          1. In the class synopsis in the documentation, the `static_assert`'s, or abbreviated versions, should also appear, so that the reader sees them there. Putting them just in the "Notes:" is suboptimal.
          2. I should try to make it so that the static asserts are fired by some `typename ENABLE` type also in the template parameter list, so that it tells the user what kinds of types can be substituted right there, and enforces the constraint immediately, not merely when the class is instantiated. (I guess I'm not even sure if this is possible due to the parameter pack, I didn't think about it, and I don't see why merely asserting when the class is instantiated isn't ok — actually I guess sometimes you want to forward declare a variant when some of the types are still incomplete, and then you cannot properly check the concepts anyway.)
          3. I should just use boost concepts check library instead of simple `static_assert`'s.
          4. I should give a name to the "minimally acceptable variant type" concept, like `boost::variant` did, and give it it's own page in the documentation. They called it "BoundedType" iirc. I guess I didn't particularly like that name, it's not very descriptive of what it is. The only name that comes to mind is "ValueType", at least if I wanted to refer collectively to the list of types, I could just say "the value types of the variant" rather than "the template parameters" as I currently do. (I might make that edit soon I guess). But actually calling the Concept "ValueType" seems bad because "value type" already has an STL meaning that isn't the same as this concept. I guess I don't see that giving a new name to the concept is better than saying "NothrowDestructible" and "not a reference". Maybe there is already some library that used this concept, I'm not sure… Maybe I didn't understand you though.

          • Robert Ramey says:

            >I see, I've watched your video now.

            >So, let me make sure that I understand exactly what you are suggesting that I do.

            >Currently the way I enforce these constraints is using `static_assert`'s here:
            https://github.com/cbeck88/strict-variant/blob/174852c68e54dae7647e7b1165186492fa27fc3d/include/strict_variant/variant.hpp#L79
            Fine
            >First thing, in the body of the class template. I thought that is a good way to enforce type constraints like this, since they will always be checked when the type is instantiated?
            Very good

            >They aren't shown in the Synopsis, but in the documentation I have a `Notes:` section which mentions the restrictions, and mentions that they are enforced by `static_assert`.
            Here is what I'm talking about.

            As documentation you've got and annotated copy of strict_variant::variant struct<T>. What happens if someone want's to specialize strict_variant::variant struct for some particular type and implement in it a totally different way?

            If you look in other well made template libraries, they are not documented in terms of the implementation but in terms of what operations the type supports. Thus the the implementation can be changed either by improvement or by specialization or some other way.

            For reference see https://htmlpreview.github.io/?https://raw.githubusercontent.com/robertramey/safe_numerics/master/doc/html/safe_numeric_concept.html

            You might want to take a look at http://en.cppreference.com/w/cpp/utility/variant and
            http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0088r0.pdf

            Unfortunately, these don't illustrate my point and is actually closer to your way of doing things.

            It seems like a simple, picky point and in the case of a simple type such as your variant it's not a huge issue.

            >Are you suggesting that:

            >1. In the class synopsis in the documentation, the `static_assert`'s, or abbreviated versions, should also appear, so that the reader sees them there. Putting them just in the "Notes:" is suboptimal.

            I expect to see the type requirements in the place where the type is described. Note that in the cppreference version there are type requirements on the page. Certain functions such as the constructor have additional requirements which are described (and ideally enforced with a static_assert). For example the default constructor creates a an object of the first types so that type must support is_default_constructible. So it is documented in the constructor documentation. Note that http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0088r0.pdf also does it in the functions under the "Requires" clause.

            >2. I should try to make it so that the static asserts are fired by some `typename ENABLE` type also in the template parameter list, so that it tells the user what kinds of types can be substituted right there, and enforces the constraint immediately, not merely when the class is instantiated. (I guess I'm not even sure if this is possible due to the parameter pack, I didn't think about it, and I don't see why merely asserting when the class is instantiated isn't ok — actually I guess sometimes you want to forward declare a variant when some of the types are still incomplete, and then you cannot properly check the concepts anyway.)

            This is what I recommend with to be implemented with static assert which uses type traits as arguments. It's not always possible without extra work, but I try to do it if possible. It's very helpful to a user who plugs in a type which can't be used to have the compile trap right at the static assert which includes an error message. It takes then 10 seconds to figure out what's wrong. If a user tries your library and gets hung up on something like this, he'll lose interest and move on. You don't want this.

            >3. I should just use boost concepts check library instead of simple `static_assert`'s.

            I don't recommend this any more. I need to update the Blincubator information. It was pretty good at the time considering what we had available. But now with C++11/14/? there are better methods. The whole "concepts" idea is in flux right now so it's not time cost effective to spend a lot of time on it. I would use static_assert just as you have done. It can be enhanced with better concepts implementation later. FWIW I am now using Paul Fultz traits library for this purpose. If it makes you feel any better, I gave him a hard time about his documentation as well. He doesn't listen though.

            > 4. I should give a name to the "minimally acceptable variant type" concept, like `boost::variant` did, and give it it's own page in the documentation. They called it "BoundedType" iirc. I guess I didn't particularly like that name, it's not very descriptive of what it is. The only name that comes to mind is "ValueType", at least if I wanted to refer collectively to the list of types, I could just say "the value types of the variant" rather than "the template parameters" as I currently do. (I might make that edit soon I guess). But actually calling the Concept "ValueType" seems bad because "value type" already has an STL meaning that isn't the same as this concept.

            >I guess I don't see that giving a new name to the concept is better than saying "NothrowDestructible" and "not a reference". Maybe there is already some library that used this concept, I'm not sure… Maybe I didn't understand you though.

            I think you're correct here. I don't think a new concept (type requirements) is in order here. Just list these specific requirements for the type parameter of strict_variant::variant. Don't create new concepts unless they are going to be reused. Note that it's OK to specify additional requirements for specific functions. This should be sufficient to address the more complex situations where requirements are not shared.

            Personally I find it helpful to factor out common "concepts" in to "base concepts". So that when I make a new type which leverages on an existing type, I can inherit all the concept documentation with the clause "refinement of base concept x". This shortens the concept documentation. The safe_numeric library does this. I also find it very helpful for clarifying my thoughts. But I don't see this applicable in your case.