[PATCH] LineIterator: Provide a variant that keeps blank lines

[Diego Novillo]

In this hunk. I'm not really sure why you needed to change the sign of the
loop. Is it because you advanced the line number above?

@@ -61,9 +72,9 @@ void line_iterator::advance() {

   // Measure the line.
   size_t Length = 0;
-  do {
+  while (Pos[Length] != '\0' && Pos[Length] != '\n') {
     ++Length;
-  } while (Pos[Length] != '\0' && Pos[Length] != '\n');
+  }

   CurrentLine = StringRef(Pos, Length);
 }

Also, skipping blank lines in your patch seems to have a counter intuitive
semantics. If I'm skipping blank lines, it means I don't want to be
returned a blank line, but I still expect the line number to be increased.
In this patch, that does not seem to happen. In fact, the test here seems
wrong:

-TEST(LineIteratorTest, CommentSkipping) {
+TEST(LineIteratorTest, CommentAndBlankSkipping) {
   std::unique_ptr<MemoryBuffer> Buffer(
       MemoryBuffer::getMemBuffer("line 1\n"
                                  "line 2\n"
                                  "# Comment 1\n"
-                                 "line 4\n"
+                                 "\n"
+                                 "line 5\n"
+                                 "\n"
                                  "# Comment 2"));

-  line_iterator I = line_iterator(*Buffer, '#'), E;
+  line_iterator I = line_iterator(*Buffer, true, '#'), E;

   EXPECT_FALSE(I.is_at_eof());
   EXPECT_NE(E, I);
@@ -59,14 +61,51 @@ TEST(LineIteratorTest, CommentSkipping) {
   EXPECT_EQ("line 2", *I);
   EXPECT_EQ(2, I.line_number());
   ++I;
-  EXPECT_EQ("line 4", *I);
-  EXPECT_EQ(4, I.line_number());
+  EXPECT_EQ("line 5", *I);
+  EXPECT_EQ(5, I.line_number());
+  ++I;

The string "line 5" is really at line 6, so I.line_number() should return
6, not 5.

I would also expect the BlankSkipping test to be failing here.


Diego.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140917/45942434/attachment.html>