BIND 10 #2373: MasterLexer's String/QuotedString state classes

[BIND 10 Development]

#2373: MasterLexer's String/QuotedString state classes
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20121120
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  libdns++                           |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  4
            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:5 jelte]:

 > master_lexer.cc:
 >
 > not entirely about this specific ticket, but in State::start() (line
 268+), any particular reason that big if-statement is not a switch?

 In this case, yes, due to this line:
 {{{#!cpp
         } else if (c == '"' && (options & MasterLexer::QSTRING) != 0) {
 }}}
 see also http://bind10.isc.org/ticket/2372#comment:9

 I guess the real point to discuss is that this case analysis is very
 big and is becoming non-understandable.  If it's concise I suspect
 none of the reviewers wouldn't even have this question.  As answered
 in #2372, I think the complexity is currently at the borderline level;
 while it's not super readable, complete refactoring would look
 overkill.

 I've added a comment so we can revisit this issue when we complete
 this method with the "NUMBER" case (#2374).

 > master_lexer_state_unittest.cc:
 >
 > for completeness, i'd add one final getToken() at the end of the
 stringEscape test, just to make sure the escaped escape doesn't somehow
 eat the rest of the input line or something (the last token 'backslash'
 isn't checked)
 > {{{
 >     EXPECT_EQ(&s_string, State::start(lexer, common_options));
 >     EXPECT_EQ(s_null, s_string.handle(lexer)); // recognize str, see ' '
 in end
 >     stringTokenCheck("backslash", s_string.getToken(lexer));
 > }}}
 >
 > Oh and it looks like some characters that are normally token separators
 aren't tested in quoted strings (like \t and parentheses)

 Okay, added these.  For the latter one, I was not sure if we wanted to
 check each character separately.  That would be better to be
 comprehensive, but it would make the test code too big for relatively
 trivial cases.  So I simply added a unified case for the rest of the
 separator characters.

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