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:

 Replying to [comment:16 jinmei]:
 > It's a linker error:
 > {{{
 > Undefined symbols for architecture x86_64:
 >   "isc::dns::master_lexer_internal::InputSource::END_OF_STREAM",
 referenced from:
 >       (anonymous
 namespace)::InputSourceTest_compactDuring_Test::TestBody() in
 run_unittests-master_lexer_inputsource_unittest.o
 >       (anonymous namespace)::InputSourceTest_compact_Test::TestBody() in
 run_unittests-master_lexer_inputsource_unittest.o
 >       (anonymous
 namespace)::checkGetAndUngetChar(isc::dns::master_lexer_internal::InputSource&,
 char const*, unsigned long) in run_unittests-
 master_lexer_inputsource_unittest.o
 > ld: symbol(s) not found for architecture x86_64
 > }}}
 >
 > This is a common problem we've seen, and we've applied the same
 > workaround (see name_unittest.cc for example).  Note that we normally
 > don't need the explicit definition - for example auth/name/query.c
 > uses Name::MAX_WIRE but we don't need this hack there.
 >
 > I've now looked into it more deeply, and found that EXPECT_EQ tries to
 > take a reference to the parameters:
 >
 > {{{#!cpp
 >   template <typename T1, typename T2>
 >   static AssertionResult Compare(const char* expected_expression,
 >                                  const char* actual_expression,
 >                                  const T1& expected,
 >                                  const T2& actual) {
 >     return CmpHelperEQ(expected_expression, actual_expression, expected,
 >                        actual);
 >   }
 > }}}
 >
 > That should be why in some tests we need the explicit definition.
 > Maybe depending on the compiler it optimizes when the expected or
 > actual is essentially constant, or it actually creates an instance of
 > the integer for the class constant (and so the test code can refer to
 > its address).

 So it was the case that there were protos but no storage allocation for
 it, so a reference could not be made to it. As you say, it looks like the
 compiler's optimizer substitutes it in other places by value.

 If we already have adopted a way of fixing this by defining it in the
 tests, then that's fine as our code is consistent.

 Otherwise, I still think it's better to have it defined in the
 `master_lexer_inputsource.cc`. The reason is that while it wastes (sizeof
 (int) + any alignment) of space, we don't have to define it for every
 program that references this storage. A user of this library will also not
 have any surprises. The space wasted should be negligible.

 But I leave the choice to you, as I merely wanted to discuss this.

 > Good point, and this is actually one of the things I intended to
 > document but forgot.  After thinking about how we handle user errors
 > in `MasterLexer`, I concluded the best is to report possible user
 > errors via return value and some kind of error code, while throwing an
 > exception for other "more-unexpected" events such as disk failure
 > after opening a file.  It's different from what other classes of the
 > library normally do, so we can discuss it if it really makes sense.
 > For now, I've added the documentation about my considerations.
 >
 > BTW, if we decide to let the exception from `InputSource` go through
 > the lexer, we should move it to the main master_lexer.h (whether it's
 > in or `MasterLexer` class or defined at the isc::dns namespace level);
 > we shouldn't disclose the existence of the InputSource class to the
 > application in the first place...hmm, in that sense `ReadError` should
 > actually be moved to master_lexer.h unless we decide to always catch
 > it within `MasterLexer`.

 I'm satisfied with the explanation you've added. While we want consistent
 code, sometimes we have to write it as the situation requires.

 I agree about the `ReadError`. I guess even `OpenError` can go to the
 `MasterLexer` class even though it's not propogated to clients of
 `MasterLexer` as `MasterLexer` is the broader class where files are asked
 to be opened.

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