BIND 10 #2371: define dns::MasterLexer class

BIND 10 Development do-not-reply at isc.org
Thu Nov 1 07:18:53 UTC 2012


#2371: define dns::MasterLexer class
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20121106
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  libdns++                           |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  5
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
  loadzone-ng                        |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => jinmei


Comment:

 Hi Jinmei

 Here's my review

 * I think I'll add this to `master_lexer_inputsource.h` even though it's
 not directly used there, as it probably belongs next to `InputSource` so
 that it's not repeated elsewhere:
 {{{
 typedef boost::shared_ptr<InputSource> InputSourcePtr;
 }}}

 * I feel `MasterLexer::close()` should have something in its names to
 indicate that it goes backward to the last source in the vector. Just
 `close()` seems less, but I struggle to think of a good name for it (maybe
 you did too). Maybe 'nestedOpen()`/`nestedClose()`? But only if you
 agree.. it's just my opinion.

 * Is it a valid situation where `getSourceName()` may be called on an
 empty MasterLexer?

 * I think "... MasterLexer will simply release.. " onwards can be changed
 to just read "... MasterLexer will simply stop using the stream." To
 release a reference may mean that a reference count is updated.
 {{{
 +    /// If it's a file, the opened file will be literally closed.
 +    /// If it's a stream, \c MasterLexer will simply release the
 reference to
 +    /// the stream; the caller can assume it will be never used in
 +    /// \c MasterLexer thereafter.
 ...
 +    void close();
 }}}

 * This seems to be incorrect:
 {{{
     /// \return A string representation of the current source (see the
     /// description)
     size_t getSourceLine() const;
 }}}

 * I have pushed some minor comment updates. Please check them.

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


More information about the bind10-tickets mailing list