Page MenuHomePhabricator

[DAGCombiner][NFC] Replace duplicate implementation flipBoolean with DAG.getLogicalNOT
ClosedPublic

Authored by laytonio on Nov 27 2020, 1:04 PM.

Diff Detail

Event Timeline

laytonio created this revision.Nov 27 2020, 1:04 PM
laytonio requested review of this revision.Nov 27 2020, 1:04 PM

I'm not really sure whether getting rid of flipBoolean() is a win.
I think i would instead suggest to refactor it to use getLogicalNOT() internally.

laytonio retitled this revision from [DAGCombiner][NFC] Replace duplicate implementation flipBoolean with DAG.getLogicalNot to [DAGCombiner][NFC] Replace duplicate implementation flipBoolean with DAG.getLogicalNOT.Nov 27 2020, 1:41 PM

I'm not sure I understand the concern here. If its just the requirement to specify a VT, maybe we should add an overload of getLogicalNOT that doesn't require one? (and maybe getNOT while we're at it) It seems reusing the VT of whatever you're flipping is probably what you want most of the time anyway.

I'm not sure I understand the concern here. If its just the requirement to specify a VT, maybe we should add an overload of getLogicalNOT that doesn't require one? (and maybe getNOT while we're at it) It seems reusing the VT of whatever you're flipping is probably what you want most of the time anyway.

I agree that this is an NFC change, i'm just not sold that more lines of code is better.

laytonio updated this revision to Diff 308115.EditedNov 27 2020, 2:22 PM

The extra lines were only because I thought it was more readable. Not needing to pass the VT would also help in that regard though.

lebedev.ri accepted this revision.Nov 27 2020, 2:32 PM

LGTM i guess, unless others have concerns.

This revision is now accepted and ready to land.Nov 27 2020, 2:32 PM

Thanks for the review! If no one else objects, could you commit this for me? Layton Kifer <laytonkifer@gmail.com>