Opened 7 years ago

Closed 7 years ago

#2506 closed task (complete)

Specialized getNextToken interface for

Reported by: vorner Owned by: jinmei
Priority: medium Milestone: Sprint-20121218
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: loadzone-ng
Estimated Difficulty: 0 Add Hours to Ticket: 4.75
Total Hours: 0 Internal?: no

Description

The task is to create a specialized getNextToken interface for the master
lexer. It should be a wrapper around the current one, specifying some specific
options and handling unexpected ends of files and lines.

This is a follow-up of #2375. More discussion can be found there.

Subtickets

Change History (12)

comment:1 Changed 7 years ago by jinmei

Whoever takes on this, please also address remaining minor issues
of #2375: http://bind10.isc.org/ticket/2375#comment:20

comment:2 Changed 7 years ago by jinmei

  • Milestone changed from New Tasks to Sprint-20121204

pushing it to the current sprint based on a jabber discussion.

comment:3 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to jinmei
  • Status changed from new to accepted

comment:4 Changed 7 years ago by jinmei

trac2506 is ready for review.

Notes for the reviewer:

  • the first two commits are to address the remaining issues in #2375 as noted in http://bind10.isc.org/ticket/2506#comment:1 They are not related to the subject of this task and can be reviewed separately.
  • The third commit (c6c7cde) is big, but it's essentially just renaming the token class and trivial adjustments due to that. I suggest reviewing this diff separately.
  • The rest of the diff is the main part of the task. It's basically a straightforward port of BIND 9's isc_lex_getmastertoken(). There are some design decisions that may not be obvious. I tried to clarify these in the documentation, so it's probably better to read it first, then the code.

No plan to add a changelog for this.

comment:5 Changed 7 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to reviewing

comment:6 Changed 7 years ago by jelte

  • Owner changed from UnAssigned to jelte

comment:7 follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to jinmei

Changes look good, I just have a few small comments on the tests:

master_lexer_unittests.cc:

eolCheck() expects a particular sequence of tokens, so it'd be nice if this got a small comment (either pointing out which tests it is intended for or which tokens it should get, i.e. the two consecutive newlines)

Regarding that, I think the 'skip the 2nd \n' can go into eolCheck :)

Oh and one additional test could be for another bad number case, where initially it does look like a number (e.g. '123abc'), for instance

diff --git a/src/lib/dns/tests/master_lexer_unittest.cc b/src/lib/dns/tests/mast
index b751da8..33266e8 100644
--- a/src/lib/dns/tests/master_lexer_unittest.cc
+++ b/src/lib/dns/tests/master_lexer_unittest.cc
@@ -369,6 +369,7 @@ TEST_F(MasterLexerTest, getNextTokenNumber) {
     ss << "\n";
     ss << "4294967296 ";        // =2^32, out of range
     ss << "not-a-number ";
+    ss << "123abc ";
     ss << "86400";
     lexer.pushSource(ss);
 
@@ -391,6 +392,11 @@ TEST_F(MasterLexerTest, getNextTokenNumber) {
     // The unexpected string should have been "ungotten".  Re-read and skip it.
     EXPECT_EQ(MasterToken::STRING, lexer.getNextToken().getType());
 
+    // Expecting a number, but see a string.
+    lexerErrorCheck(lexer, MasterToken::NUMBER, MasterToken::BAD_NUMBER);
+    // The unexpected string should have been "ungotten".  Re-read and skip it.
+    EXPECT_EQ(MasterToken::STRING, lexer.getNextToken().getType());
+
     // Unless we specify NUMBER, decimal number string should be recognized
     // as a string.
     EXPECT_EQ("86400",

(this does not necessarily test anything extra, but given the way tokens are recognized I do believe this is a potentially useful addition)

comment:8 in reply to: ↑ 7 Changed 7 years ago by jinmei

Thanks for the review.

Replying to jelte:

master_lexer_unittests.cc:

eolCheck() expects a particular sequence of tokens, so it'd be nice if this got a small comment (either pointing out which tests it is intended for or which tokens it should get, i.e. the two consecutive newlines)

Okay, I added some more comments.

Regarding that, I think the 'skip the 2nd \n' can go into eolCheck :)

Yeah I was aware of that, and I don't remember why I chose not to
do so - maybe it was because it didn't look a job of "check". But
with some more detailed comments that now doesn't seem to be a concern
anyway, so I unified it in eolCheck().

Oh and one additional test could be for another bad number case, where initially it does look like a number (e.g. '123abc'), for instance

[...]

(this does not necessarily test anything extra, but given the way tokens are recognized I do believe this is a potentially useful addition)

Okay, added.

comment:9 Changed 7 years ago by jinmei

  • Owner changed from jinmei to jelte

comment:10 follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to jinmei

ok, this can be merged :)

comment:11 in reply to: ↑ 10 Changed 7 years ago by jinmei

Replying to jelte:

ok, this can be merged :)

Thanks, merge done, closing.

comment:12 Changed 7 years ago by jinmei

  • Add Hours to Ticket changed from 0 to 4.75
  • Resolution set to complete
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.