[boost] Numeric conversions lib comments

From: Gennadiy Rozental (gennadiy.rozental_at_[hidden])
Date: 2004-01-04 01:44:20

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.

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.

1. Why bounds<>::smallest() returns 0 and not 1, for integral types? Doesn't
it supposed to return smallest positive value?

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.

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.

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&?

5. IMO Thee should be simple direct way to make range checking debug only
operation

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.

2. Could you please explain logic behind subranged_Unsig2Sig,
subranged_Udt2BuiltIn, subranged_BuiltIn2Udt implementations
3. ct_switch4 implementation:  could we make this implementation more
efficient? If is_case0 is true why would we need the rest checks?
4. How would trivial_converter_impl will work if one convert from const int
to int? Should it work?
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.

Docs
------------------------------------------------------------------

Definitions:

1. Why "object representation" is sequence of bytes, while "value
representation" is set of bits?
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.
3. "intrinsic value" definition: why are you using unsigned characters
instead of bytes here?
4. After "abstraction" definition: "Abstraction is just an abstract
operation" - it's like saying salt is salty
5. In C++ arithmetic types section: operation {} needs to be defined
6. In C++ arithmetic types section: " The integer types are required ....
" - what does this statement mean
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)
8. In numeric types section: definitions for integer and floating types are
unclear to me. Does everybody know what whole and real means?
9. In Range and Precision: "The set of representable values of type 'T'. is
...." - representable where?
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.
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.
12. In "Standard (numeric) Conversions": "Promotions, both Integral and ...
is not changed with the conversion" - should be promotion?
13. In "Standard (numeric) Conversions":  what does "in a sequel" mean?
14. In "Standard (numeric) Conversions":  "Floating to Floating conversions
are defined only if 'N' is representable" - where?
15. In "Subranged Conversion ... ": why are you using word intersection in
one place and symbol & in another?
16. In "Subranged Conversion ... ":  "Notice that for S->T, the adjective
subranged applies to 'T'" - why not to conversion?
17. Discussion list address is wrong

Type requirements and ....

18. ... in order to use the default Trunc<> policy - Instead of putting it
into brackets

Header boost/numeric/conversion_traits.hpp

19. In template class is_subranged description you use source (first letter
lowercase) and Target (first letter uppercase).

Header boost/numeric/converter.hpp

20. range_check_result used but not defined
21 "If the user supplied a UserRangeChecker policy, is this policy which
...". "is this" looks incorrect
22. "This function is externally supplied by the RawConverter policy
class.". "externally supplied" term is unclear.
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?

Header boost/numeric/converter_policies.hpp

24. "This stateless non-template policy class": what do you mean by
non-template? And later you mention template policy class. What do you mean
by that?

Code
------------------------------------------------------------------

1. Copyright needs to be updated.
2. Why do you name namespace convdetail/boundsdetail and not detail?
3. IMO for_both maybe named if_both
4. Why ct_switch4 and not ct_switch3. After all you have only 3 case clauses
5. for_round_style and similar selectors I would name select_round_style
e.t.c.
6. Why do you need get_ in get_is_subranged?
7. GetRC_Sig2Sig_or_Unsig2Unsig I would name GetRC_SameSig
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)
9. Why do you need first three typedefs in rounding converter
implementation?
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.
11. Why each enums are defined in separate header? Did you intend them to be
used separately from rest of implementation?

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?

Congratulation to the author for the solid work. Hope my comments were
productive and may help to make it even better.

Regards,

Gennadiy.

begin 666 traits_test.zip
M4$L#!!0````(`!HC(C"/2\^#D_at_P``*!B```/`"0`=')A:71S7W1E<W0N8W!P
M"@`@```````!`!@`!,L/1A+1PP&]5[AIC=+#`;AIMCK\T,,!U5QK;]M&%OT>
M(/_AUOU0*:#MV 46"UDQX#IRUUB_$,D%]@6"(D?R;"D.,3.THP;][SM//D1*
MIB2:S*:(2\[CWC-G+L_E2,X]/H;>91]./W[\V8$K1",O"@C<))C!I>?[F(2>
MN*!3+PR]\.C]N^-C^1<F3V+ PN.(8B\$<1U3\HP#%,"!Q\3]@0,OF#^!-V4D
M3#@*EQ 1>/$H]2*^!/0UIH_at_Q%"ACA )>Q"%&P1%<1$M(&)(F/0Y+DE @+Q%0
MS'[/O#\@NL",81(!)VJX,.&3> E<XF)DQH4K!#/1[ F#<4)CHHT^(8JF2YA+
M',:]!$H2#C.$G&P=_ D)].09"> <^TC@$18IXAZ.1+=P[86A=(H1.ZI M2 !
MGBV5&9\$PE84R.8`,T[Q-.%(CQ"+UOT"FP'E*&LU_at_3C*L&?ZQ&"/9SY?/*9L
MI9Z$$QSY82+MJOW)C$OZ*)X_<6,J9?O]NQ_-G"$F`CSR%N>%MH47X3C?)%<8
MS?,M?!DC',U(ONT9^9S0?(L7S_at_D5J*3]K!F&4R+\'ON,!SCB1T]QWIGM33_at_.
M,5^NZ8TIBBGQ$6.$'ON>-5(>&"4+1+%_[)/H&5&YERZG'N9LC6$[7@!S9R'Q
MN+O 7WE"T2OC&9Y'-8<F`7>G"0XYKCL#,Y<E4^I%<Q2D*ST^AE_DJ*,)8KP\
MER/SP^6$A.N6FXWR/89<CA9QZ'&+)V$XFH,:.1_at_D$1:VY- 9]1;HA=#?!P-U
MSQ+,T5D>DW 4H!F.$/QR?S^>N+</-^[-]>WU1/P4M^/K?X[@]&,9CO!^'&)6
MBHF#K/N_1&[-,T8O<M!!]2!.O8C-"%UL&#.C1 ??FOZ8Q.YK8[PX#I<NGFT8
M_at_A:QCN(#'9ZS`,W =7^Y_W)S<??YTG5%8TR]^<*#IX R3F+1@*(`SPKA?*"8
M?D)AC"@[\E?<Y3M/3:_=OLA;(!9[/@+&`S_at_K-RNL51TF_"IZ;I>/GR=@MWSR
MA!@"XUZ)6<)0`%*5482HQY&+OL;(YR_at_P#]]PXHS/M<@^1B'^'2G=8GB!0X^F
MEG"DFF4J00L4<8]C$CFR;:F\,&$3>R'^0SM#7SF*Y!-NE,X&\] //<9_at_XH"^
M&)\#XS3Q.2R6;O[1D_at_NJ.XE3_"PS)=2?M*HHM6?FM:7VI J5R6V82O8^)3*!
MJ,V2F4P_L9+RF)(848X1`S(#Y/E/D DH3!%_02A2IN1HXP8\I?6(4^R#3! L
MS>_7W%AG>J/MQJD=E2YDJ]YVT!&2AM!/ZV+H)Y/+#.S/HZOKNY%[>7_WV^C+
M^/K^KC?QZ!QQ9TP2ZB-GHC?,N=5<.&,\C^SU8\#3YF3Z105#'_[]_AWL_"<W
M.=VN=7$W+" ]AZ+G;XI+J1K"S& P)21TA]#+`3U7(^ ,_A1_]T%=`[2)^Q+D
M6IC-'JP_at_WH_HII:V^G265[B-CQ(%PCZ:4R]T_:')IT9=55?1M8M$GP,]$Y.K
M='T?A.5%:0>N]B$L[]IRE7N<RWQ]%X15"/+6O.U(6(5KRULF?7G:9)[(I-5/
MM0;$:E"N8Y)F0=UA7P=@1L*0O,B7!OEJ`AYC`@L#3TO_#/M 4:C5WV03\P;P
MATXY\D:E$-$MK<AL<&83#D4S0I&CSHFIO1G^BH+#%QS(0Y":B2/&D1?H]'0H
M7R94<X I\GFX/#)P[^XG(S@\/(?)Q9=?1Q-G?/_XY7*DDTLYJ=B781SQO[H<
MP(%BBP,I*PZD6\*):Z\%[$@&L,QCQ+77CDVBLM%<.O!#COG^V08X%DT%G!^V
M@:.OE)D*.% 7CG1^\A<!:$\XS;&CT+PM.[7A2.<_GWY/["@T7;.C4F!.ZVK#
ML1.;A1.09!JB[P9.2*(YY#!U#.=V>1WMM%E;Q$X2<&VZ#IRK0O2\!3MKX6R5
M)FK34_<YKRW*Y1Q1.V.]B006$T2GO*QDAQVPO%EJZ)27E;S0`2]KDT+MY[J%
MC- !EK7IH ,L:W-!4_'21")HBI=FLH#5OT;?^/;(!#OBV1PY>QX7-)Z.3E,5
MYX5N^2D?&!J/GSU/#(WSL^>182.>#LX,'>#9>&CH`,_&4T-3\=/4L:$I?AH[
M-]0DJ(5TL0N8-\L5W3)3/CK4S%OMG!TZ9*9\>&@X9O8[/=1\O-LY/K0.9M/Y
MH74PFPX0S<1,0R>(9IAI[@@A'['OZ0BQ`YXW2PN=\U-QA.B6G_(10N/IZ(A5
M<83H@)^-1XB->#HX0G2 9^,1H@,\&X\03<5/4T>(IOAI[ A1DZ 6TL4N8-[R
M"-$A,^4C1)?,E(X0'3)3/D+4S%CM'"%J/M[M'"%:![/I"-$ZF$U'B&:>IH:.
M$,TPLV="4'X**/3O'[:6!+8$T+SP=\: %?ON&# "WQD#5M1;9J LY 8`5 !H
M1[PW,+ ]@)[ZY4DRTPN5O\QI[K7?/O1KZW@[N')^UX,KZWH38;.7EC?!SI[Z
MK5GK4L"W1="\@G?'@97P#CDP&MX=!U;$V^:@K.*;$.RA5E:8SE?D:YU0E=7=
M`FLKOY1UO"UJ=E3R1F)G+REOA* ]M3Q'7I>"OA.,YE6]8S:LM'?-AM'WCMFP
M(M\)&V6E?Q7&'II6D+#=-;]]B-N_W1<PMI6<RNK?7%#ME0*:V[$]\X!B:,?/
M6>6_[\OV0QC;(0%L[S]C86?_.>7O:OU6\CM;O]'ZKM9O1;[=]9?5?;/_S4_A
M]O[+TMVN_[(LM^N_K,?&_TY?K1C_1@'!V46(&UC_6O]U%?AJU\]$FQ'@+=TW
MK;^=K#Z3WVY6GZIO)ZO/Q+?%U5=I[P;W;4AOB^ZKE+=%]U7"N^_>[ZF[5[M^
M#_*Z[*;%!9&N`R'K"B(J"W/IXCX^6<2>K"VG:PS-, H#(#,U"6"UC$6IAILJ
M(Y66_%-%(Y0)EMFH*CRE9SQAX5%6);05AW01)>%$U^P0",7D$/M853N4I0=-
M(:3%%$>J=H4N=CC+BB =YJH_at_V2)(ZZI1Y4IYF/)-%4V)+,,DK%1U3==V4+4'
MY2ZSP5G[^?MWID#)"D_OWWV3HVR!$?E_6?VKNHX5@*)U,%#E0RKK7%6;*A2V
M,G_RIMA*X:MJ*U65KO)6JOI5=&;F4LZ Y8N!90/RA4YRA;_R%LQ."0OV:F6 
MVB\QCZ578L"?Q:)<."M\Q>U#@Z*YJLM%TG)JX*WN%\PH6<!$U8X<KXNXM$Q8
MNNOK:FNM;'_Y(8*Q`_JGJCTS\T*&7',C2\&X(*O)&*K.7K>ES>6F%PS+CSNJ
M]V4[9*FUB$3%LF]K@[U0IBN+J6LVK@:T,CF-EN+D23Z(JJ=+O'CF#G.>G!2*
M4UB M1LI84PY?\VNE7LSQ<E-MQ;!QO%*F.K:<39Z6%:@5)4KU:JNROT(FX?*
MAM)E90!'-83]7"NZDF5"A:V81(',$P4[J[M_7BP)-QF-)^YO%S>/(_?J>G3S
MN7?G+5"AKINN3GGYM]'EW]W;T7A\\>L(>N(_;6\PD!,&@V<O3!!\^I3Z*W;T
MP7F]P-1!>8T',!PJ_at_G'0F_2/Y/;T^K+MP,EWC=.N5YW(N>>#@9K]HX2H6N 3
M:'O$U85<>Q7+JVG]"$:6`V&T:+62G5?M]G>,K')4M1).DW\\;!]->B/SK/=E
M.)GV(F_]_YMXJES75K8KHJF2DII&TU#2X21SIT^H+GP,ES9T"F^:.[UC5H20
M*I.YYE6SE(D?1/,SP8%*[V:T._48ZL'#!_@$'Z&_[OWK83"882H6!N/JM",&
M,"06$<!D)0746ZE]&:ZVOK&>J\D9MLNFH)(*EUX0^]I;:6#^]6_=F(J7N[7F
M;+Y<-\ DOWQW]KA#+WVQ6S]@FG:K$GJF^/%H<N%>/=Y=NFK&Y<5XU"MMO3DP
MK1;:5>DZ#L7E$PD#1)D:E<93NC%C)[V<9*]W/B6,Q90$XN;$A-2Z=X*T9/(P
M?7>*/4R'[HDC3(+=7?GC#$ L<!V2DPS*^+0:RQY375D->/U2"BL>2HLKRU05
MG8?"NEY/^J)CC)V<U3!<93>M%IW9+G!VJDC+FU;3LFK6B_at_9-P*DNQ'BV'\G"
MW4 [L56JA^I.%:26*,TNRWK;'X?BKD3S4+@1X^0I\5N&Q1;\_O#^'98EP;.Z
MX*JY)P^ PMB31S_ O_X#??BF=0!D_6DI.PF72J]N&.(Q13Z6*M3334:?W! O
MI+#D/JXY'PP"/!>-)Q^MU.MBFRDB=?W)Y&'UL(T?KR>C'AS<::OY*L9&Z.24
M`_/TE;9'<G-R,K1I9[6H6.%#7 =*Q0.*'W-6_-._at_E=\+MW[,KQC:KR4+WU&:
MS\GM!SDZ5%@)?B%>U1 5,2R-25OSG16(/#SW_at_J!G&#2:=?MP<S$9;1(O)V?.
M?!.M35+$$QJI7JN)_P-02P$"&0`4````" `:(R(PCTO/@Y(,``"@8@``#P``
M`````````" `````````=')A:71S7W1E<W0N8W!P4$L%!@`````!``$`/0``
'`.,,````````
`
end