search

conjure-cp / conjure-oxide

Mozilla Public License 2.0
8 stars 16 forks source link

Removing remove_empty_expression #427

Open YehorBoiar opened 4 days ago

YehorBoiar commented 4 days ago

In the PR #370 @ozgurakgun pointed our that rule remove_empty_expression doesn't make much sence becuase the same thing should happen in a higher priority rule apply_eval_constant.

Should we just get rid of that rule?

niklasdewally commented 4 days ago

There are a few rules left over that are duplicates total/partial evaluators, so I agree with this.

Once #401 is merged, you should be able to find these quite easily.

YehorBoiar commented 2 days ago

I checked #401. Now, new choose_rewrite would warn us about rules that are applicable, but not being applied as they are of a low priority.

So, I can just go through all warn messages that we are getting from that function right now. See what rules are never being applied. Open a PR where we would discuss removing those kind of rules. @ozgurakgun @niklasdewally what do you think?

niklasdewally commented 2 days ago

I checked #401. Now, new choose_rewrite would warn us about rules that are applicable, but not being applied as they are of a low priority.

So, I can just go through all warn messages that we are getting from that function right now. See what rules are never being applied. Open a PR where we would discuss removing those kind of rules. @ozgurakgun @niklasdewally what do you think?

Perfect plan!

ozgurakgun commented 2 days ago

sounds good to me too