[boost] Re: Numeric conversions lib comments

From: Fernando Cacciola (fernando_cacciola_at_[hidden])
Date: 2004-01-06 11:03:09

Gennadiy Rozental <gennadiy.rozental_at_[hidden]> wrote in message
news:bt8cmd$ujn$1_at_sea.gmane.org...
> Hi,
>
> I know that numeric conversion library review is over and it's got accepted.
> But since I was on vocation at the time let me give these comments now to
> the version that was submitted. I did not have time to look through review
> comments, so there may be repetitions.
>
Thanks for your detailed review!

> Design
> ------------------------------------------------------------------
>
> In most part proposed design looks reasonable. I have only one major IMO
> concern.
>
> Issue: (Major) Why traits are template parameter of converter?
>  I've expressed my opinion in the matter before: Traits - as name suggest
> could not be policy. IOW should not be template parameters. Traits are type
> specific and an ability to supply them independently to the primary type in
> most cases leads to invalid/too complicated design.
>    Traits are not only template parameters of public converter class but
> many other classes. IMO all of them should use S and T as template
> parameters instead.
>
I have to disagree.
Traits are used as policies in too many designs, including the STL itself, and I don't see a
problem with that.
I agree that traits classes are type specific, but they are used as policies precisely because
of that: if the user of a parametrized facility wants a different treatment for a given type,
she can't change *the* traits for the type, all she can do is pass in a different traits as a
policy.
If this were not a policy, users would be stuck with the definiton of the default traits
class.
I agree that this opens a window to all sort of problems in case a user plugs in an invalid
traits, but I think that should not constitute a reason for fixing the traits into the
converter.
As always, "trust the programmer"

> 1. Why bounds<>::smallest() returns 0 and not 1, for integral types? Doesn't
> it supposed to return smallest positive value?
>
You're right... nobody missed this one :-)

> 2. boost::numeric::converter is part of public interface and have 7 (should
> be 6) template parameters. Such number of parameters asks for named template
> parameters facility.
>
Right indeed...
I just couldn't get bcc5.5/5.6 to work with the new ntp so I leaved it off until we get an ntp
facility working with most compilers.

> 3. All converter policies required to be stateless. Why?
>    I don't understand this requirement. Moreover, I believe in some (rare)
> cases I may want statefull one. For example Let say I want to keep track of
> location where risky arithmetic operations performed. So OverflowHandler
> needs hold on to this position and report it in exception.
>
I see.
The motivation was to avoid having instances of the policies as part of the converter state.
That is, to make it leaner.
But I see your point and after all most compilers will effectively optimize away those policies
which are actually empty classes, so I think I'll remove the requirement.

> 4. converter<>::convert converts only rvalue. Why?
>   IMO we need second operation that works with lvalues. What if I want to
> convert int& into char&?
>
This requires the target lvalue as a parameter instead of a return value.
It could be done yes, but I don't see the reason.
Why would want to do that?

> 5. IMO Thee should be simple direct way to make range checking debug only
> operation
>
Good point.
Though out of range errors are usually not bugs, meaning that the checks really shouldn't be
debug only, there should be a at least a simple way to remove range checking all toghether for a
release build for those rare applications working with constrained data.

>
> Implementation
> ------------------------------------------------------------------
>
> 1. I am not sure that it is really the issue. But if it is it would be major
> concern. Whole implementation is based on the following logic
>
> template<...>
> some_type_generator
> {
>      typedef some-complex-type-generator case1;
>      typedef some-complex-type-generator case2;
>      ....
>
>     typedef ct_switch( type_selector ) case1 or case2 .... type;
>
> // for example
> //   typedef typename
> //
> mpl::apply_if<use_internal_RC,InternalRangeCheckerQ,UserRangeCheckerQ>::type
> //        RangeChecker ;
> };
>
> I wonder whether all caseN types get instantiated. If yes, it may be quite
> wasteful.
>
I may need to revise this on a case by case basis, but I used metafunction "quoting"
to instantiate only those which are actually used.
What you may be seeing is the situation when the "cases" are types extracted from
template parameters, as in:

  typedef typename X::type case0 ;
  typedef typename Y::type case1 ;

In this situation, case0, case1, etc are already instantiated inside X and Y (here are just
"promoted" into the scope of the template class)

> 2. Could you please explain logic behind subranged_Unsig2Sig,
> subranged_Udt2BuiltIn, subranged_BuiltIn2Udt implementations

Sure,

subranged_Unsig2Sig:
--------------------

First, let's see the relevant sections of the standard:

(a) 3.9.1/3 Fundamental Types, Unsigned Integer Types
"....each signed integer type has the same object representation as its corresponding unsigned
integer type..."

(b) 3.9.1/3 Fundamental Types, Unsigned Integer Types
"...The range of nonnegative values of a signed integer type
is a subrange of the corresponding unsigned integer type, and the value representation of each
corresponding signed/unsigned type shall be the same..."

(c) 3.9.1/7 Fundamental Types, Integral Types
"The representation of integral types shall define values by use of a pure binary representation
system"

(d) 18.2.1.2/7 numeric_limit::digits
"For built-in integer types, the number of non-sign bits in the representation"

AFAICT, there are _only_ 3 binary representation schemes for signed integers: two's complement,
one's complement and sign-magnitude. (the standard even gives these 3 schemes as examples of
valid representations).
All these 3 reps have in a common that 1, and only 1 bit is needed for the sign.
It follows from this that if N is the number of non-sign bits used to represent a signed
integer, N+1 bits are used to represent the corresponding unsigned integer.

(b) says that "...the range of nonnegative values of a signed integer type
is a subrange of the corresponding unsigned integer type...", but,
it follows from (a) and (c) that this subrange is not _any_ subrange but
exactly half the range of the unsigned type minus 1.
Furthermore, half range is covered by exactly half bits, thus, the range of a signed type
completely contains the range of an unsigned type iff: s+1 >= u, were s and u are the non-sign
bits used by the signed and unsigned types respectively.

NOTE: The following contains a good survey of representation schemes:

http://poincare.math.swt.edu/Classes/2358NumberRepresentations.pdf

subranged_Udt2BuiltIn
subranged_BuiltIn2Udt
---------------------

User Defined Numeric Types can be almost anything, so, in order to compare the ranges of a UDT
and a built-in I had two choices: come up with some general way to compare ranges of arbitrary
numeric types "at compile time" (using only constant expression), or fix the relation
arbitrarily.
I choseen option 2 and assumed that any UDT ha wider range than any built-in type.

Anyway, this assumption has been noted already during the review and I agreed to give the matter
a deeper thought to see if UDTs can be better supported.

> 3. ct_switch4 implementation:  could we make this implementation more
> efficient? If is_case0 is true why would we need the rest checks?

Good point. Yes, I think it can be improved

> 4. How would trivial_converter_impl will work if one convert from const int
> to int? Should it work?

Yes, it should.
Top-level cv-qualifiers are removed from template parameters, so a conversion
from "const int" to "int" is actually _seen_ by the converter as a trivial conversion
from just "int" to "int"

> 5. Separation of policies in make_converter_from type generator looks
> unclear tome. In an case: why would I want to use it, instead of using
> converter directly? NTP looks better solution to me.

hmmm, I agree that it looks odd....
I also agree that NTP is a better solution... but as I said above, NTP technology is not well
supported by some compilers.

>
> Docs
> ------------------------------------------------------------------
>
> Definitions:
>
> 1. Why "object representation" is sequence of bytes, while "value
> representation" is set of bits?

Because the standard said so that way (3.9.4).
...well, almost :-)
The standard uses "unsigned char" instead of "byte", which is not necesarily the same,
so I have to correct this.

> 2. "Typed value" definition: how it could by determined only by set of bits
> (value representation)? What about signed unsigned values? They have the
> same value representation as a set of bits.

The "value representation" of an object contains _all_ the bits that form the value of that
object.
In the case of signed/unsigned types, the sign bit is part of the value representation; it is
the "sign scheme" which makes sense of the value rep to extract the value with its proper sign.

> 3. "intrinsic value" definition: why are you using unsigned characters
> instead of bytes here?

Because the object rep is given in terms of unsigned chars instead of bytes (though the docs
incorrectly state the opposite)

> 4. After "abstraction" definition: "Abstraction is just an abstract
> operation" - it's like saying salt is salty

Yes it is... and it is an important qualification worth mentioning it IMO.

> 5. In C++ arithmetic types section: operation {} needs to be defined

Does it? It the classic set-by-extension nomenclature.

> 6. In C++ arithmetic types section: " The integer types are required ....
> " - what does this statement mean

The complete statement is
"The integer types are required to have a binary value representation"
It means that integer types must be binary.

> 7. In C++ arithmetic types section: you give some advice at the end - does
> it belong here?
>    Actually I completely disagree with second part of what you recommend.
> IMO one should use unsigned values to model non-negative values. For once
> it's better reflect design. And also it's more safe and efficient. Would I
> use signed value as an container index, then before accessing value by index
> I need to not only check that it does not exceed size of the container but
> also check that it is non negative - twice as much work and easy to forget.
> Also in most cases errors with "negative"  index are easier to catch.
> Instead of more or less safe (begin-1) memory access, one would get (begin +
> max_int)
>
Well, we won't get into this endless argument, so I'll remove the advice which anyway doesn't
belong there.

> 8. In numeric types section: definitions for integer and floating types are
> unclear to me. Does everybody know what whole and real means?

"whole" means unfragmented, it is used in math to refer to integer numbers
"real" is the standard math name for the set of numbers that floatint-point formats represent.

> 9. In Range and Precision: "The set of representable values of type 'T'. is
> ...." - representable where?

In T

> 10. Why precision is defined the way it is defined? What value this
> definition brings? There seems to be two different concepts combined undef
> the same hood.

Because the term has different meanings when applied to integer or floating-point types.
I've split the definiton in those two meanings.

> 11. Exact, ... section: " Notice that a representation is an operation ...
> ". Actually "Rounding" is an operation, which has direction and error. This
> name seems more logical. So we are getting representation my means of
> applying rounding operation to an abstract value.

It is true that technically computers do not "represent" numbers.
But it is also true that "to represent" is in itself an operation, or action if you prefer:
namely, to "give a concrete form to something". It is just not a "computer" operation.

You may argue that I shouldn't describe it as an opertation if it is not actually produceable by
a computer... however,

These definitions uses the artifact of "trampolining" out form the computers world over the
abstract math world. Things such as "abstract values", and the "abstaction" operations do not
really exist neither in a computer, yet they are used througout the document.

"to represent" is an (hypotetical) operation in the context of the document because it refers to
the (hypotetical) action of giving numbers (abstract entities) a concrete form within a
computer.
"to abstract", for instance, is the opposite operation.

"rounding" is also an operation; in fact, one produceable by a computer; but is conveys a
different meaning so I can't use it in place of "representation".

> 12. In "Standard (numeric) Conversions": "Promotions, both Integral and ...
> is not changed with the conversion" - should be promotion?

I used the word "conversion" there precisely to indicate that a promotion
is a kind of conversion which doesn't change the value.

> 13. In "Standard (numeric) Conversions":  what does "in a sequel" mean?

The corrrect expression is "in the sequel" (note the "the"); it means: in what follows.

> 14. In "Standard (numeric) Conversions":  "Floating to Floating conversions
> are defined only if 'N' is representable" - where?

in the target type.

> 15. In "Subranged Conversion ... ": why are you using word intersection in
> one place and symbol & in another?

Good point.. I'll change this either way.

> 16. In "Subranged Conversion ... ":  "Notice that for S->T, the adjective
> subranged applies to 'T'" - why not to conversion?

Good point... it applies to 'T' w.r.t 'S', that is, to the conversion.

> 17. Discussion list address is wrong
>
Oh, I'll fix it.

>
> Type requirements and ....
>
> 18. ... in order to use the default Trunc<> policy - Instead of putting it
> into brackets
>
Oh.. I don't understand this comment.

> Header boost/numeric/conversion_traits.hpp
>
> 19. In template class is_subranged description you use source (first letter
> lowercase) and Target (first letter uppercase).
>
OK

> Header boost/numeric/converter.hpp
>
> 20. range_check_result used but not defined
OK

> 21 "If the user supplied a UserRangeChecker policy, is this policy which
> ...". "is this" looks incorrect

Not to me, but, maybe... I'm not a native English speaker. Anyone?

> 22. "This function is externally supplied by the RawConverter policy
> class.". "externally supplied" term is unclear.

Is "supplied externally" better? Again, I can't tell which form is more correct.

> 23. "Internal Member Functions: ...., but they can be called separately for
> specific needs.". If they could be called by end-user why do you name them
> internal?
>
Because they are alrady called by the "usual" public interface.

> Header boost/numeric/converter_policies.hpp
>
> 24. "This stateless non-template policy class": what do you mean by
> non-template?

That this policy is not parametrized. It is not a template class but a regular class.

> And later you mention template policy class. What do you mean
> by that?
>

That this policy _is_ parametrized. It is a template class, not a regular class.

> Code
> ------------------------------------------------------------------
>
> 1. Copyright needs to be updated.
OK

> 2. Why do you name namespace convdetail/boundsdetail and not detail?

To avoid conflict with other stuff in boost::detail...
Sometimes different boost libraries declare clashing names in boost::detail.
This situation is not covered by the regression test; it shows up when both libraries are used
toghether.

> 3. IMO for_both maybe named if_both

To me, if_both would look like a function returning true if both expr are true, false otherwise;
but this is not what for_both does.

> 4. Why ct_switch4 and not ct_switch3. After all you have only 3 case clauses

Because it is conceptually dispatching 4 possibities. The fourth clause is ommited
because it drops by default.

> 5. for_round_style and similar selectors I would name select_round_style
> e.t.c.

It was like that originally, but then I thought about sorthening the names a little.

> 6. Why do you need get_ in get_is_subranged?

Because there are classed named "subranged" which are the predicates themselves,
while "get_subranged" is the metafunction selecting one of those classes.

There is one "get_is_subranged" though, which is the top-level selector.
I could have use "is_" al over the other subselectors and predicates, but I
removed the "get" to make the names a little shorter.

> 7. GetRC_Sig2Sig_or_Unsig2Unsig I would name GetRC_SameSig

But "Sig" alone doesn't tell whether Sig2Unsig and viceversa is also included.
These names require a delicate balance between terseness and clarity.

> 8. Name you are using for some identifiers looks like mix of 2 styles (
> GetRC_Sig2Unsig ). I think we may better following boost naming
> recommendations (like get_rc_sig_2_unsig)

Most underscores used by this naming scheme are unnecesary.
I prefer to use them not to separate words (Capitals are used for that) but to separte concepts.
For example, GetRC_xyz were xyz are specific range checkers.

> 9. Why do you need first three typedefs in rounding converter
> implementation?

Becasue they are all used in the code

> 10. Why do you need separate files for most of implementation details?
>    Majority of implementation presented as one tiny header in numeric
> directory that present public interface implemented by inheritance from
> implementation located in separate header in details directory. I do not see
> real advantage in such separation.

Because those details are used by interface headers which can be used separatedly.

> 11. Why each enums are defined in separate header? Did you intend them to be
> used separately from rest of implementation?
Yes.
The "mixture" traits classes, and is_subranged trait class is intended to be used separately.

>
> Tests
> ------------------------------------------------------------------
>
> 1. Why test_helpers are .cpp and not .hpp?
> 2. All your test ask to be written in terms of test cases. Do you need help
> to convert them to use unit_test_framework?
> 3. Many of your tests ask to be converted to use new test case template
> facility. I attached I test that I reworked in this direction. Do you need
> help with that?
>
OK, Great! I'll see to use you version.

>
> Congratulation to the author for the solid work.
>
Thanks!

>Hope my comments were productive and may help to make it even better.
>
They sure did!

Best,

Fernando Cacciola