Page MenuHomePhabricator

[libTooling] Change `after` range-selector to operate only on source ranges
ClosedPublic

Authored by ymandel on Oct 15 2020, 7:34 AM.

Details

Summary

Currently, after fails when applied to locations in macro arguments. This
change projects the subrange into a file source range and then applies after.

Diff Detail

Unit TestsFailed

TimeTest
270 mswindows > lld.ELF/invalid::symtab-sh-info.s
Script: -- : 'RUN: at line 4'; c:\ws\w16-1\llvm-project\premerge-checks\build\bin\yaml2obj.exe --docnum=1 C:\ws\w16-1\llvm-project\premerge-checks\lld\test\ELF\invalid\symtab-sh-info.s -o C:\ws\w16-1\llvm-project\premerge-checks\build\tools\lld\test\ELF\invalid\Output\symtab-sh-info.s.tmp.o

Event Timeline

ymandel created this revision.Oct 15 2020, 7:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 15 2020, 7:34 AM
ymandel requested review of this revision.Oct 15 2020, 7:34 AM
tdl-g requested changes to this revision.Oct 15 2020, 7:49 AM

Just one comment about the tests.

clang/unittests/Tooling/RangeSelectorTest.cpp
196

If this helper function took an "expected" parameter I might consider it self-explanatory, but as it is, it's extremely non-obvious what it does without reading the body in detail--which means the tests that use it are not particularly readable either. (Each test reads like "here's the input data, make sure something unspecified is true about it.") Specifically, this function searches for a reference to the variable "y" and ensures that after() finds the character after it. So at a minimum, document that.

I'm also trying to decide if this helper is too similar to the implementation--tests should not just restate what the production code does, they should find some other way to validate the behavior. Do you think this is sufficiently different from the prod implementation to be meaningful? If so, that's fine. If not, maybe the helper should just take an expected byte offset, be renamed to "afterYHasByteOffset", and then each test would be a bit more readable?

Up to you.

This revision now requires changes to proceed.Oct 15 2020, 7:49 AM
ymandel updated this revision to Diff 298392.Oct 15 2020, 8:30 AM

cleaned up test code

ymandel added inline comments.Oct 15 2020, 8:33 AM
clang/unittests/Tooling/RangeSelectorTest.cpp
196

I reduced the helper to just getting the spelling location at the given offset. the rest of the code I've inlined into the tests. That helper is still substantially less complicated than what happens in the production code, and I don't see any better way to find the location, unless we want to do an offset from, say, the braces of the function. that avoids macro-related calculations, but also disconnects from the var. So, I think it's just a tradeoff rather than an improvement.

tdl-g accepted this revision.Oct 15 2020, 1:42 PM
This revision is now accepted and ready to land.Oct 15 2020, 1:42 PM