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

[Justin Bogner]

On Wednesday, September 17, 2014, Diego Novillo <dnovillo at google.com> wrote:

> 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);
>  }
>

Since the line can have zero length now, we have to check the condition
first. The old way would step over the newline at Pos[0].


> 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:
>

I think you've misread something here - the line number is increased when
we skip blank lines. The behaviour when SkipBlanks is true is unchanged
from without this patch.


> -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.
>

No, the string "Line 5" is really line 5. Note that an extra newline was
added to the test, so line 4 is now blank.


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

Why?


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