BIND 10 #172: cc channel - use JSON

[BIND 10 Development]

#172: cc channel - use JSON
----------------------+-----------------------------------------------------
 Reporter:  larissas  |        Owner:  stephen                                       
     Type:  task      |       Status:  reviewing                                     
 Priority:  major     |    Milestone:  05. 3rd Incremental Release: Serious Secondary
Component:  msgq      |   Resolution:                                                
 Keywords:            |    Sensitive:  0                                             
----------------------+-----------------------------------------------------
Changes (by jelte):

  * owner:  jelte => stephen


Comment:

 Again, the comments that I agreed with and fixed right away are left out
 in this response, for some I had some extra comments, see below.

 >
 > src/lib/python/isc/cc/tests/Makefile.am
 > ===
 > Changes for this ticket - OK
 >
 > Remark: The change commented out the running of data_test.py,
 session_test.py, and test.py.  Should these files be deleted?

 salvaged and readded session_test and data_test, and deleted test.py
 (which only had wire format tests)

 >
 > Test: equals
 > ---
 > There is a comment "why does EXPECT_EQ not work?".  EXPECT_EQ seemed to
 work for me - was it a problem with the version of Google test? (I am
 using 1.3.0)
 >

 indeed, it works here too, changed it back (if it pops up again, we'll
 know where to look, and EXPECT_EQ does make it much clearer in my opinion)

 >
 > src/lib/cc/data.h
 > ===
 > Remark: The choice of constants for the "types" enum: the names
 "string", "list" and "map" - although aptly named - could be confused with
 type declarations.  Leave for now, but perhaps change in the future?
 >

 If you have suggestions, i'd gladly hear them, I also had the plan of
 renaming the namespace (data), which has a related problem, so we can
 nicely fit that into one ticket :)

 > Comments for toWire() method suggest that there ought to be a parameter
 named omit_length. The comment should have been removed when the method
 signature was changed.
 >
 > The comment "// pure virtuals, every derived class must implement these"
 is not correct - it is followed by a number of non-pure-virtual
 definitions.
 >

 Right, i moved another pure virtual there and changed the // comment to
 /// \name, so that it is grouped on the same level as the rest.

 > Remark: The fromJSON() methods include a "throw" in the exception
 signature which is not the case with other methods (e.g. intValue()).
 What is the standard for BIND-10 - should method signatures include the
 list of exceptions that can be thrown?
 >

 There is no standard, I suggested this at one point (I think when i wrote
 the original version of this), but there didn't seem to be much support. I
 would still like to see it though, but we should do it everywhere or
 nowhere at all. Nice point for a discussion (and action after it's
 resolved).

 >
 > src/lib/cc/data.cc
 > ===
 > count_chars_i()
 > ---
 > This will return an incorrect result if the input argument is negative.
 >

 ok, added a check, which reverses it and adds 1 to the result (for the -).
 Only thing where this could fail is the value -0, but i'm not sure whether
 we should account for this.

 >
 > from_stringstream_number()
 > ---
 > It would be consistent to declare d_i inside the "if" clause starting
 "if (in.peek() == '.') {" as that is the only place it is used.  (However,
 this is a very minor point.)
 >
 > If both "d" and "is_double" are initialized on declaration, so should
 the other automatic variables.
 >
 > An input of something like "1e30" will cause an integer overflow when
 the statement "i = i * p" is executed.  Converting the result to a double
 should be considered.
 >

 That wouldn't solve the general problem, would it? I'm thinking (as said
 in the comment), that we should consider a general Number class.

 > Is the string "1.e2" considered a valid double?  The code will throw a
 JSON error if this is encountered.
 >

 no, that's invalid

 > A string of the form "8.  2" will be be interpreted as 8.2
 >

 The way I read the JSON spec, that's actually supposed to happen. I
 noticed that this is a lenience that not all parsers have, so we may
 reconsider (it's pretty lenient in general, and can recover from certain
 classes of bad input data)

 > There is an assumption that the decimal part of a string representing a
 floating point number has only one digit.  This leads to odd results, e.g.
 "8.23" is interpreted as 10.3 (= 8 + 23/10).
 >

 doh, fixed (keep dividing until < 1, i'm thinking there's a more efficient
 way for this :)

 >
 > MapElement::set
 > ---
 > There is a magic number of 255 in the method.  This should be defined as
 a constant somewhere.
 >

 Oh, yeah, actually, now that you mention it, that check was imposed by the
 old wireformat (which had only 1 byte for the tag length), which isn't a
 problem anymore, so now tags can be as long as you want. Removed the
 check.

 >

 <skipping the more general comments, will handle those later>

 >
 > src/lib/cc/session.cc
 > ===
 >
 > Remark: There is little internal documentation in this module.  The
 purpose and implementation need to be documented: the class names suggest
 that the code could be reused elsewhere, but the lack of documentation
 will make it difficult to do so.

 agree, but out of scope for this ticket. Feel free to create a new one for
 that.

 >
 > src/lib/config/module_spec.cc
 > ===
 >
 > General: this file contains a "getType_string()" function that returns a
 string representation of the Element::types enum, and a getTypes_value()
 function that does the inverse.  These functions would be better made
 static methods of the the Element class.  (They have got out of sync with
 the class; there is no translation for the "null" type.)
 >

 ok done, renamed them to nameToType and typeToName though

 I think that's it for the review, made 4 commits to address your comments:
 r2295, r2296, r2297 and r2304

 As said above, this does not include that specific set of general comments
 about both files, I'm not sure whether we shouldn't make a separate task
 of this (which would be way way smaller than this one)

-- 
Ticket URL: <http://bind10.isc.org/ticket/172#comment:10>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development