This is an archive of the discontinued LLVM Phabricator instance.

[ConstantRange] Simplify unittests after getSetSize was removed
ClosedPublic

Authored by MaskRay on Apr 14 2019, 1:11 AM.

Diff Detail

Event Timeline

MaskRay created this revision.Apr 14 2019, 1:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2019, 1:11 AM

Please just revert those two commits.

nikic added inline comments.Apr 14 2019, 1:24 AM
unittests/IR/ConstantRangeTest.cpp
53–59

It would seem more natural to write this as a do/while loop:

APInt N = CR.getLower();
do {
    TestFn(N);
} while (++N != CR.getUpper());
1465–1466

Should use ForeachNumInConstantRange twice here (and drop the comment about sizes above).

The commit message was

The last use of this helper method was removed by rL302385.

Which is clearly not true, as this commit removed more uses of that method, that weren't testing the method itself, but using it. Therefore i'm questioning the whole removal. Moving it into a static function in tests is another, unrelated matter.

That commit didn't remove more uses of that method. The last library use was removed two years ago and no use has been added since then. I deleted it, and then realized there was a unittest relying on it.

and the way the test was rewritten to avoid it looks more complicated than it was with the function, at least to me..

Please just revert those two commits.

I agree that the last commit made it more complex than it should. I apologize for that. So I created this revision. However, "more complicated than it was with the function" is your opinion and I'm not convinced of that judgement. So, I'd like to get more input from others, especially those who removed the last few library uses.

MaskRay marked 2 inline comments as done.Apr 14 2019, 1:35 AM

The commit message was

The last use of this helper method was removed by rL302385.

Which is clearly not true, as this commit removed more uses of that method, that weren't testing the method itself, but using it. Therefore i'm questioning the whole removal. Moving it into a static function in tests is another, unrelated matter.

That commit didn't remove more uses of that method. The last library use was removed two years ago and no use has been added since then. I deleted it, and then realized there was a unittest relying on it.

That is precisely my point, it was still used, in test.
As you have just wrote, that wasn't considered when dropping the method.

and the way the test was rewritten to avoid it looks more complicated than it was with the function, at least to me..

Please just revert those two commits.

I agree that the last commit made it more complex than it should. I apologize for that. So I created this revision. However, "more complicated than it was with the function" is your opinion and I'm not convinced of that judgement. So, I'd like to get more input from others, especially those who removed the last few library uses.

The commit message was

The last use of this helper method was removed by rL302385.

Which is clearly not true, as this commit removed more uses of that method, that weren't testing the method itself, but using it. Therefore i'm questioning the whole removal. Moving it into a static function in tests is another, unrelated matter.

That commit didn't remove more uses of that method. The last library use was removed two years ago and no use has been added since then. I deleted it, and then realized there was a unittest relying on it.

That is precisely my point, it was still used, in test.
As you have just wrote, that wasn't considered when dropping the method.

When my first commit was made, it was indeed used in the unittest (I don't index /(clang|lld|llvm)/(test|unittests)/ with my language server so I missed the references in unittests, it seems I should), but I soon fixed that (the second commit). It wasn't elegant and I saw your complaint, so I created this revision.

and the way the test was rewritten to avoid it looks more complicated than it was with the function, at least to me..

Please just revert those two commits.

I agree that the last commit made it more complex than it should. I apologize for that. So I created this revision. However, "more complicated than it was with the function" is your opinion and I'm not convinced of that judgement. So, I'd like to get more input from others, especially those who removed the last few library uses.

nikic accepted this revision.Apr 14 2019, 1:47 AM

LGTM. The end result here looks better to me than the original getSetSize() based code, so we'd want to do these changes even independently of that removal.

unittests/IR/ConstantRangeTest.cpp
1462–1463

The "These loops..." etc part of this comment should be dropped, as it's no longer relevant with the current implementation.

1466

I think we should drop these two asserts (or at least put them inside ForeachNumInConstantRange, but I don't think it's worthwhile to have them).

This revision is now accepted and ready to land.Apr 14 2019, 1:47 AM
MaskRay updated this revision to Diff 195052.Apr 14 2019, 2:16 AM
MaskRay marked 2 inline comments as done.

Apply nikic's suggestion

This revision was automatically updated to reflect the committed changes.