BIND 10 #2371: define dns::MasterLexer class

[BIND 10 Development]

#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      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Thanks for the review.

 Replying to [comment:9 muks]:

 > * 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;
 > }}}

 In this case I don't oppose to it; after all, `MasterLexer` is
 effectively the only user of inputsource.h, so it's not different much.

 If it's a general case, I'll be a bit more careful though.  Having a
 shared_ptr definition in a header file means we add another dependency
 to boost to all files that include this header file.  In general, I'd
 like to minimize such dependency.  So, unless we are sure the shared
 pointer shortcut will be widely used, I'd rather keep it a local
 definition.

 > * 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.

 I don't have a strong opinion, but I see your point.  Since the
 sources conceptually constructs a stack, how about "pushSource()" and
 "popSource()" instead of "open()" and "close()"?

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

 Hmm, good question.  So you mean it's better to throw in such a case?
 Maybe so...but after checking how BIND 9 uses the return value, I'm
 not really sure...in some places it check if it's NULL or not and
 change the behavior accordingly.  So there may actually be some usage
 when we want to call it without any source.  On the other hand, I
 generally don't like returning a magic value like NULL to implicitly
 indicate something (like the "queue is empty").  So, maybe at the
 moment we throw an exception in this case with noting we may revisit
 it if we see a valid case later?

 I also think the same discussion applies to getSourceLine().

 At the moment I didn't touch the code.

 > * 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();
 > }}}

 Okay, changed.

 > * 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.

 These looks fine.  Thanks.

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