Remove unused PostTc types from TuplePat
Needs RevisionPublic

Authored by osa1 on Feb 2 2017, 8:22 AM.

Details

osa1 retitled this revision from to Remove unused PostTc types from TuplePat.Feb 2 2017, 8:22 AM
osa1 updated this object.
osa1 edited the test plan for this revision. (Show Details)

Did you conclude that the big comment you removed was wrong? Maybe include your reasoning in the commit message?

osa1 added a comment.EditedFeb 3 2017, 1:59 AM

@rwbarton I think so, DsUtils.hs:826 would be an error if SPJ's comment was wrong, and that line was committed in 2009.

I realized that field is incorrectly set as [] in the code. Use cases are mostly routine recursive traversals (e.g. when zonking all the fields), or just passes LHS to RHS. There were one actual use case, but the value used is actually derived using the first field in DsUtils.hs:826.

So in short, SPJ's comment has to be correct, but I don't understand the details. I'll add my reasoning to the commit message.

There's a test failure but I doubt it's related with this patch. I can't reproduce it locally. Perhaps @bgamari would know?

In the commit title you say the field is unused. I don't think that can be what you mean, since it is used as an argument to vanillaConPattern, mkPrefixConPat, and mkTupleTy.

Rather, I think you mean that we can recompute it when we need it. But that's just what the comment that you deleted says you can't do:

-- You might think that the PostTc id Type was redundant, because we can
-- get the pattern type by getting the types of the sub-patterns.

Yet you also say the comment is correct. So I'm lost.

I realized that field is incorrectly set as [] in the code.

Where? Does this change also fix a bug? I thought it was supposed to be a refactoring.

There's a test failure but I doubt it's related with this patch. I can't reproduce it locally. Perhaps @bgamari would know?

Yes, setnumcapabilities001 fails sometimes on OS X, unfortunately.

osa1 added a comment.Feb 4 2017, 12:08 AM

Sorry for my confusing comments,

Rather, I think you mean that we can recompute it when we need it. But that's just what the comment that you deleted says you can't do:

Indeed that's what I mean. I think _that part_ of the comment is wrong.

Yet you also say the comment is correct. So I'm lost.

So there're actually two comments. One is the comment that says we shouldn't delete this field, and the other one is SPJ's comment. O mean the former is wrong, and the latter is right.

Where? Does this change also fix a bug? I thought it was supposed to be a refactoring.

I'm not sure if this fixes any bugs. It was just that the removed field was being incorrectly initialized by the code before. My guess to why this wasn't causing any trouble is because the field is not used after some transformations (that may be enforced by the PostTc type family, I'm not sure).

June 14: I'm not sure this comment is right; the sub-patterns will be wrapped in CoPats, no?

Have you verified that this is the case, @osa1?

bgamari requested changes to this revision.Mar 2 2017, 2:10 PM

@osa1 says,

I'll be getting back to that code soon for the or patterns work, give me more time :)

Requesting changes until that happens.

This revision now requires changes to proceed.Mar 2 2017, 2:10 PM
austin resigned from this revision.Nov 9 2017, 11:33 AM