BIND 10 #2369: InputSource helper class for MasterLexer

[BIND 10 Development]

#2369: InputSource helper class for MasterLexer
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  muks
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20121106
  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      |
-------------------------------------+-------------------------------------
Changes (by vorner):

 * owner:  vorner => muks


Comment:

 Hello

 Replying to [comment:14 muks]:
 > > Also, in this ticket, I don't see the use of the `saveLine()`. Does it
 make any
 > > sense to save the line and then rewind all the way to the beginning?
 Then the
 > > line numbers will be wrong, won't they? Should the `saveLine()` and
 `compact()`
 > > be a single method, so the `saveLine()` saves both the line and the
 position?
 >
 > Yes it would be a problem if we rewind to a position before where the
 saved line starts. IIRC we thought that as it's an internal class to
 `MasterLexer`, the lexer uses it knowing this behavior. In any case, it
 may be worth merging `saveLine()` and `compact()`, and perhaps call it
 something more meaningful like `mark()`.

 Yes, please. I see no use case for doing one without other, so they'd be
 called
 both every time, which is long, or one of them forgotten, which would be
 wrong.

 > > The problem is with `ungetAll()`. As I mentioned above, I don't know
 if it is
 > > really needed. But the `ungetChar()` can have the parameter ‒ the
 character to
 > > return, then we could have buffer only for the returned characters.
 >
 > We don't use it this way (at least in my reading of it), i.e., as a pure
 push-back buffer where we only read back what we `ungetChar()`.

 Yes, I know. But if there was such parameter, we would not need to
 remember the
 old parameters and could pass the responsibility onto the caller.

 > > And, usually, we we'll be on top of files. Rewinding a file is
 possible ‒ we
 > > just start reading it again. So we can have a special-case for the
 file
 > > version, that does not buffer, but rewinds the underlying file.
 >
 > As long as the lexer `compact()`s when starting to scan new tokens, we
 don't need to worry about the buffer size I guess.

 I was just looking for a more elegant solution. But this is probably fine
 too.

 Also, to the code.

 The file that does not exist, I guess ``/videokilledtheradiostar`` will
 never
 exist there. But it might be confusing. I'd suggest something like
 ``does-not-exist``, which is more obvious about the purpose, even while
 more conservative.

 I think the content of the string should be read from the actual file, so
 we don't have the same content twice:
 {{{#!c++
 TEST_F(InputSourceTest, file) {
     const char* str =
         ";; a simple (incomplete) zone file\n"
         "\n"
         "example.com. 3600 IN TXT \"test data\"\n"
         "www.example.com. 60 IN A 192.0.2.1\n"
         "www.example.com. 60 IN A 192.0.2.2\n";
     size_t str_length = strlen(str);

     InputSource source(TEST_DATA_SRCDIR "/masterload.txt");
     checkGetAndUngetChar(source, str, str_length);
 }
 }}}

 Would it be possible to test the invalid read exception by some trick? I
 don't see one now (maybe something like a socket file could do the trick,
 but I'm not sure).

 And I'm thinking, what happens if we copy an opened file stream? Should we
 make the input source class non-copyable just to make sure? (well, with
 the const name, it is non-assignable at least, so for consistency too).

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