c2rust icon indicating copy to clipboard operation
c2rust copied to clipboard

`check_unique` refactor

Open kkysen opened this issue 3 years ago • 5 comments

Refactored all the unique and non-unique assert!ions into check_unique, which is much more concise and avoids the ton of repetition there was previously. It also adds more context to the error message when it fails.

kkysen avatar Sep 16 '22 06:09 kkysen

One downside I see to this approach is that in some cases it forces the node names in the assertion to be out of order compared to the graph, which makes it slightly harder to match the two up. I think we could get the best of both worlds with a macro along the lines of assert_unique!(pdg, [a, !j, !b1, !b2, !c1, !c2, d2]);, or something like this:

let [a, j, b1, b2, c1, c2, d2] = unique_flags([a, j, b1, b2, c1, c2, d2]);
assert!(a && !j && !b1 && !b2 && !c1 && !c2 && d2);

spernsteiner avatar Sep 16 '22 16:09 spernsteiner

One downside I see to this approach is that in some cases it forces the node names in the assertion to be out of order compared to the graph, which makes it slightly harder to match the two up. I think we could get the best of both worlds with a macro along the lines of assert_unique!(pdg, [a, !j, !b1, !b2, !c1, !c2, d2]);, or something like this:

let [a, j, b1, b2, c1, c2, d2] = unique_flags([a, j, b1, b2, c1, c2, d2]);
assert!(a && !j && !b1 && !b2 && !c1 && !c2 && d2);

See #668. That solves this issue I think by putting the expected unique value where we create the Node.

kkysen avatar Sep 16 '22 16:09 kkysen

I think we could get the best of both worlds with a macro along the lines of assert_unique!(pdg, [a, !j, !b1, !b2, !c1, !c2, d2]); I don't think we should add a macro if we don't need to.

or something like this:

let [a, j, b1, b2, c1, c2, d2] = unique_flags([a, j, b1, b2, c1, c2, d2]);
assert!(a && !j && !b1 && !b2 && !c1 && !c2 && d2);

This wouldn't give good error messages, though.

kkysen avatar Sep 16 '22 19:09 kkysen

This whole PR is superseded by the approach in #668, right?

spernsteiner avatar Sep 16 '22 22:09 spernsteiner

This whole PR is superseded by the approach in #668, right?

Yeah, pretty much.

kkysen avatar Sep 16 '22 23:09 kkysen