BIND 10 #2372: define State class for MasterLexer, base part

[BIND 10 Development]

#2372: define State class for MasterLexer, base part
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20121120
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  libdns++                           |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  6
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
  loadzone-ng                        |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => jinmei


Comment:

 Hello

 First, it does not compile for me:

 {{{
 ../../../../src/lib/dns/master_lexer.cc: In static member function ‘static
 const isc::dns::master_lexer_internal::State&
 isc::dns::master_lexer_internal::State::getInstance(isc::dns::master_lexer_internal::State::ID)’:
 ../../../../src/lib/dns/master_lexer.cc:221:1: error: control reaches end
 of non-void function [-Werror=return-type]
 }}}

 Also, I'd like to clarify ‒ does the single transition mean producing one
 token? So you start after each new token?

 Looking at this enum, I know it is correct code, but it looks unusual and
 possibly fragile to me (if it is re-ordered, it will do strange things).
 Would
 it be better to do it like:
 {{{#!c++
     enum Options {
         NONE = 0, //< No option
         INITIAL_WS = 1 << 0, ///< recognize begin-of-line spaces
         QSTRING = 1 << 1, ///< recognize quoted string
         NUMBER = 1 << 2 ///< recognize numeric text as integer
     };
 }}}

 The comment seems to be missing „in“:
 {{{
 The initial transition takes place a static method of the base class,
 }}}

 I think the concrete state classes can be inside the anonymous namespace
 as
 well. They are defined inside the C++ file and don't escape outside, so
 there's
 no need to pollute the namespace. It also should allow the compiler to do
 more
 optimisations, knowing the code won't be used from other module directly.

 Some sessions seem to produce no tokens at all (and return NULL). Is the
 old
 token reset, so the caller knows there's no token? Or how the caller knows
 not
 to look into the token?

 The `start` method contains a loop with huge if-else-if-else sequence. It
 could
 be written as a switch, which could be more readable. Is there a reason to
 use
 the if-else way? Maybe some extension in future?

 In the tests, you seem to re-fill the string stream after reading from it.
 Is
 it OK? Can it be used in the way that you read and write from the same
 stringstream? What happens if you get to the end while reading (so it
 returns
 EOF) and then refill it? Is it allowed by the stream? Couldn't the lexer
 get
 confused?

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