This is an archive of the discontinued LLVM Phabricator instance.

[lit] Script to automate use of %(line-n). Use in CodeComplete tests.
ClosedPublic

Authored by sammccall on Dec 16 2022, 7:12 AM.

Details

Summary

Tests where the RUN-lines/CHECK-ed output refer to line numbers in the test
file are a maintenance burden, as inserting text in the appropriate place
invalidates all the subsequent line numbers.

Lit supports %(line+n) for this, and FileCheck supports [[@LINE+N]].
But many existing tests don't make use of it and still need to be modified.

This commit adds a script that can find line numbers in tests according to a
regex and replace them with the appropriate relative-line reference.
It contains some options to avoid inappropriately rewriting tests where absolute
numbers are appropriate: a "nearby" threshold and a refusal by default to
replace only some matched line numbers.

I've applied it to CodeComplete tests, this proves the concept but also are the
single worst group of tests I've seen in this respect.
These changes are likely to hit merge conflicts, but can be regenerated with:

find ../clang/test/CodeCompletion/ -type f | grep -v /Inputs/ | xargs ../llvm/utils/relative_lines.py --verbose --near=20 --pattern='-code-completion-at[ =]%s:(\\d+):' --pattern='requires fix-it: {(\d+):\d+-(\d+):\d+}'
`

As requested in https://reviews.llvm.org/D140044

Fixes https://github.com/llvm/llvm-project/issues/59553

Diff Detail

Event Timeline

sammccall created this revision.Dec 16 2022, 7:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 16 2022, 7:12 AM
sammccall requested review of this revision.Dec 16 2022, 7:12 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
sammccall updated this revision to Diff 483547.Dec 16 2022, 8:02 AM
sammccall edited the summary of this revision. (Show Details)

add bug link

hokein accepted this revision.Dec 19 2022, 1:14 AM

Thanks! The change looks good to me (I assume our internal lit support part is completed). (I think we can do a cleanup in clang/test/Index as well).

llvm/utils/relative_lines.py
18

This variable is not used in anywhere, and not print out in --help mode. I think it is more useful to move it to the above file comment, at least it is shown in --help mode.

19

nit: find command is missing -type f.

21

\\d should be \d?

32

nit: the default value 0 basically does nothing, set a reasonable number, 20?

This revision is now accepted and ready to land.Dec 19 2022, 1:14 AM
hokein added inline comments.Dec 19 2022, 1:46 AM
llvm/utils/relative_lines.py
46

it would be nice to make the script more robust (capture and bailout the UnicodeDecodeError exception)-- there are some test files that contain invalid utf-8 characters

95

looks like we modify the file more eagerly -- if there are no matching pattern, we will replace the content as well (most cases are ok, I saw the CRLF => LF change, I think we'd better only overwrite the content if there are matching patterns).

sammccall marked 6 inline comments as done.Dec 19 2022, 4:53 AM
sammccall added inline comments.
llvm/utils/relative_lines.py
18

oops, I meant to add it as the help epilog. done now

21

Indeed - I'm surprised neither python nor shell requires the backslash to be escaped...

95

Oops, this is processing as text rather than binary. Added ugly bytes conversions everywhere.

(Rewriting the file after no changes seems silly, but it should be a no-op and it shows up bugs like this)

This revision was landed with ongoing or failed builds.Dec 19 2022, 5:19 AM
This revision was automatically updated to reflect the committed changes.
sammccall marked 3 inline comments as done.

looks like you forgot to upload the new diff.

looks like you forgot to upload the new diff.

The changes were trivial/mechanical so I wasn't planning on another round of review, but was waiting for the tests to finish before landing.
Sorry for confusion, happy to address any comments.