BIND 10 #2375: implement MasterLexer getNextToken() and ungetToken()

[BIND 10 Development]

#2375: implement MasterLexer getNextToken() and ungetToken()
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  vorner
  jinmei                             |                Status:  closed
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20121204
  medium                             |            Resolution:  fixed
                  Component:         |             Sensitive:  0
  libdns++                           |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  5
            Defect Severity:  N/A    |           Total Hours:  10.73
Feature Depending on Ticket:         |
  loadzone-ng                        |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Responding to non blocking points...

 Replying to [comment:15 vorner]:

 > > So, to me, the question to be asked is opposite: whether there's any
 > > advantage of handling the exception here.
 >
 > > Maybe you don't like it because the total amount of code of this
 > > version is even bigger than just using try-catch.  But at least it
 > > simplifies the main code of `getNextToken()` and doesn't have the
 > > performance issue with any type of try-catch implementation unless an
 > > exception is actually thrown.
 >
 > I'd be opposed to that approach not only because the amount of code is
 larger.
 > I find it much less readable than the try-catch one. The try catch
 clearly
 > states the intention ‒ „Do the transitions and if anything goes wrong,
 try
 > returning back to previous state“. While the version with guard would
 read
 > something like „First, make sure we return to the previous state
 afterwards. Do
 > the transitions. And, oh, well, I changed my mind. Don't return to the
 previous
 > state.“

 I admit simplicity/readability is sometimes in the eye of the
 beholder, so I won't fight about which version is more readable
 further.  But I'd like to point out that if try-catch were to be
 considered a better practice if we want to make sure cleaning up
 something on exception, we'd have to use the idiom where we generally
 use the RAII style cleanup (there are many of them in our code).  The
 idiom with a guard object is just one form of such usual convention.

 > > I agree your version is more logical.  But I don't think it wise to
 > > just ignore the fact that the BIND 9 version has been working for
 > > quite a long time against many very unusual or even broken inputs.
 > > So, while I'm okay with resetting last_was_eol in the end, I suggest
 > > you really think about any possible problematic cases where the
 > > difference can cause, rather than just dismissing it "because it's
 > > more logical".  I'd also ask this simple question (why it doesn't
 > > presever-restore last_was_eol) at bind10-dev (some core BIND 9
 > > developers subscribe to it).  Actually I suspect it's less likely
 > > you'll get an answer, but we can then at least know there's not a
 > > simple reason for it, if any.
 >
 > I did write the email. But I still don't agree with your point. It can
 be seen
 > from the other side too. One of the reasons why bind10 was started was
 the
 > fact that bind9 was hard to maintain and read. And, supposing it was not
 a
 > forgotten revert to previous state (which didn't manifest, therefore the
 > difference wouldn't matter), but a sideeffect that was actually used
 somewhere,
 > it would be one of the places where it would be really hard to read and
 needed
 > to be eliminated, not copied. Bind10 is not supposed to be simple
 rewrite of bind9
 > to C++.

 I have no objection to these points per se, so either I was not clear
 about my point or you misunderstood it.  I'm not saying we should
 blindly follow what BIND 9 does.  My point is that (unless some very
 trivial case like NULL pointer dereference bugs) there can always be
 some specific reason about long-standing code that's proven to work
 even if it doesn't look logical at first glance.  So my point is that
 we should at least try to understand whether there could be such
 reasons or it was just overlooked but didn't actually cause a trouble,
 by thinking about various corner cases and how it would work with or
 without the "illogical" version of the implementation.  It's different
 from immediately dismissing such code as being not logical just by
 seeing it, which I opposed.  If, after such efforts, we are confident
 that there shouldn't be any valid reason of the original behavior, we
 should do it differently with confidence; if we actually find a tricky
 corner case that would be handled by the seemingly illogical
 implementation, then we can discuss whether to follow the behavior or
 even totally revise the design so all cases can be handled in a more
 intuitive way.

 > > Another example is an unbalanced '(' followed by EOF.  We first need
 > > to detect the error and then detect EOF.
 > >
 > > So, we should clearly do something, at least if we want to be
 > > compatible with BIND 9.
 >
 > Now I see the problem.
 >
 > I don't know if we need to be compatible with bind9 in these very
 internal interfaces. But it sounds reasonable to report the error and then
 EOF, because the caller needs to know no more tokens (or errors) will get
 out. So I changed it.

 This is probably a good practical example of the previous point.  We
 do not necessarily have to follow what BIND 9 does literally.  But if
 we do things differently we first need to understand why the original
 version is implemented that way and check whether skipping it causes
 any visible problem.

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