This is an archive of the discontinued LLVM Phabricator instance.

[Rewrite][NFC] Add FIXMEs and tests for RemoveLineIfEmpty bug
ClosedPublic

Authored by jdenny on May 2 2019, 2:05 PM.

Details

Summary

I'd like to add these comments to warn others of problems I
encountered when trying to use RemoveLineIfEmpty. I originally
tried to fix the problem, but I realized I could implement the
functionality more easily and efficiently in my calling code where I
can make the simplifying assumption that there are no prior edits to
the line from which text is being removed. While I've lost the
motivation to write a fix, which doesn't look easy, I figure a warning
to others is better than silence.

I've added a unit test to demonstrate the problem. I don't know how
to mark it as an expected failure, so I just marked it disabled.

Diff Detail

Repository
rL LLVM

Event Timeline

jdenny created this revision.May 2 2019, 2:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2019, 2:05 PM
Herald added a subscriber: dexonsmith. · View Herald Transcript
jdenny updated this revision to Diff 208795.Jul 9 2019, 1:28 PM
jdenny retitled this revision from [Rewrite][NFC] Add FIXME about RemoveLineIfEmpty to [Rewrite][NFC] Add FIXMEs and tests for RemoveLineIfEmpty bug.
jdenny edited the summary of this revision. (Show Details)

Rebase, and add a unit test to demonstrate the bug.

Hi @jdenny, thanks for the warning! I think having the test disabled is fine - the main upside I see is that we won't check it fails over and over on our CI systems.
Could you please mention the test/reproducer in those FIXMEs?
Otherwise LGTM.

clang/unittests/Rewrite/RewriteBufferTest.cpp
17 ↗(On Diff #208795)

I think in case you really wanted to have an "XFAIL", you could have just used EXPECT_NE here.

Hi @jdenny, thanks for the warning!

Hi. Thanks for the review.

I think having the test disabled is fine - the main upside I see is that we won't check it fails over and over on our CI systems.

In an inline comment, you also mentioned the alternative of replacing EXPECT_EQ with EXPECT_NE. Neither solution is the XFAIL I'm used to (from lit for example).

I chose disabling purely because I saw some other unit tests do this. What do people normally recommend for llvm unit tests?

Could you please mention the test/reproducer in those FIXMEs?

Will do.

In an inline comment, you also mentioned the alternative of replacing EXPECT_EQ with EXPECT_NE. Neither solution is the XFAIL I'm used to (from lit for example).

I'm not aware of any assertion with this semantics in Google Test.

jdenny updated this revision to Diff 213197.Aug 3 2019, 12:04 PM

Note the reproducer in the implementation's FIXME. The other FIXME points there, so I figure it's best not to duplicate the note.

Note the reproducer in the implementation's FIXME. The other FIXME points there, so I figure it's best not to duplicate the note.

@jkorous, is this change sufficient? Thanks.

jkorous accepted this revision.Aug 15 2019, 11:31 AM

LGTM. Thanks!

This revision is now accepted and ready to land.Aug 15 2019, 11:31 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 15 2019, 2:18 PM