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      |
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => jinmei


Comment:

 Hi Jinmei

 Replying to [comment:10 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'll not add it to `InputSource` in this case. If it gets used anywhere
 else (unlikely) we can add it then.

 > > * 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()"?

 These sound good to me.

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

 We better leave it then, in case these can be called on empty
 `MasterLexer`, as otherwise the caller may try to catch and work around
 that exception.

 The rest look fine. You can do the `open()`/`close()` rename and directly
 merge it to master.

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