Replace some uses of forall_operands by ranged-for #5790

tautschnig · 2021-01-26T21:38:40Z

In some (but not all!) cases of forall_operands we do not actually need the
iterator.

Each commit message has a non-empty body, explaining why the change was made.
n/a Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
n/a The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
n/a My commit message includes data points confirming performance improvements (if claimed).
My PR is restricted to a single feature or bugfix.
n/a White-space or formatting changes outside the feature-related changed lines are in commits of their own.

codecov · 2021-01-26T22:19:38Z

Codecov Report

Base: 78.38% // Head: 78.34% // Decreases project coverage by -0.03% ⚠️

Coverage data is based on head (e4aaa48) compared to base (794f6f7).
Patch coverage: 69.83% of modified lines in pull request are covered.

❗ Current head e4aaa48 differs from pull request most recent head 7cb1f5b. Consider uploading reports for the commit 7cb1f5b to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #5790      +/-   ##
===========================================
- Coverage    78.38%   78.34%   -0.04%     
===========================================
  Files         1647     1645       -2     
  Lines       190343   190355      +12     
===========================================
- Hits        149196   149131      -65     
- Misses       41147    41224      +77

Impacted Files	Coverage Δ
src/analyses/invariant_set.cpp	`0.00% <0.00%> (ø)`
.../goto-instrument/accelerate/acceleration_utils.cpp	`2.23% <0.00%> (ø)`
src/goto-programs/goto_convert_side_effect.cpp	`94.50% <0.00%> (ø)`
src/jsil/parser.y	`0.00% <0.00%> (ø)`
src/solvers/flattening/boolbv_case.cpp	`0.00% <0.00%> (ø)`
src/solvers/flattening/boolbv_cond.cpp	`0.00% <0.00%> (ø)`
...olvers/flattening/boolbv_constraint_select_one.cpp	`0.00% <0.00%> (ø)`
src/util/simplify_expr_if.cpp	`39.15% <0.00%> (ø)`
src/goto-programs/interpreter_evaluate.cpp	`33.88% <14.81%> (ø)`
src/analyses/goto_rw.cpp	`66.59% <50.00%> (ø)`
... and 139 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

peterschrammel

lgtm

peterschrammel · 2021-01-26T22:27:53Z

src/goto-symex/goto_symex_is_constant.h

-      forall_operands(it, expr)
-      if(!is_constant(expr.op0()))
-      return false;
+#if 0


martin-cs

A few questions about style but nothing blocking.

src/goto-programs/adjust_float_expressions.cpp

src/goto-programs/goto_clean_expr.cpp

src/goto-symex/goto_symex_is_constant.h

hannes-steffenhagen-diffblue

LGTM

hannes-steffenhagen-diffblue · 2021-01-28T16:05:17Z

src/analyses/custom_bitvector_analysis.cpp

@@ -693,9 +693,11 @@ bool custom_bitvector_domaint::has_get_must_or_may(const exprt &src)
  if(src.id() == ID_get_must || src.id() == ID_get_may)
    return true;

-  forall_operands(it, src)
-    if(has_get_must_or_may(*it))
+  for(const auto &op : src.operands())


FWIW this is just std::any(src.operands.begin(), src.operands.end(), has_get_must_or_may)

kroening · 2021-02-17T23:24:56Z

src/jsil/parser.y

@@ -132,9 +132,9 @@ procedure_decl: TOK_PROCEDURE proc_ident '(' parameters_opt ')'
              {
                symbol_exprt proc(to_symbol_expr(parser_stack($2)));
                code_typet::parameterst parameters;
-                forall_operands(it, parser_stack($4))
+                for(const auto &op : as_const(parser_stack($4)).operands())


This deviates from the pattern elsewhere -- why the as_const?

Because .operands() would trigger a(n unnecessary) write on the underlying irep, temporarly breaking (probably non-existing) sharing.

In some (but not all!) cases of forall_operands we do not actually need the iterator.

tautschnig added the cleanup label Jan 26, 2021

tautschnig requested review from allredj, chrisr-diffblue, hannes-steffenhagen-diffblue, kroening, martin-cs, peterschrammel, romainbrenguier and smowton as code owners January 26, 2021 21:38

tautschnig force-pushed the forall-operands branch from 99e6f8a to 9d0e497 Compare January 26, 2021 21:39

tautschnig changed the title ~~Replace some uses forall_operands by ranged-for~~ Replace some uses of forall_operands by ranged-for Jan 26, 2021

peterschrammel approved these changes Jan 26, 2021

View reviewed changes

src/goto-symex/goto_symex_is_constant.h

forall_operands(it, expr)

if(!is_constant(expr.op0()))

return false;

#if 0

Copy link

Member

peterschrammel Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

tautschnig assigned kroening Jan 27, 2021

martin-cs approved these changes Jan 28, 2021

View reviewed changes

src/goto-programs/adjust_float_expressions.cpp Show resolved Hide resolved

src/goto-programs/goto_clean_expr.cpp Show resolved Hide resolved

src/goto-symex/goto_symex_is_constant.h Show resolved Hide resolved

tautschnig force-pushed the forall-operands branch from 9d0e497 to e01fcff Compare January 29, 2021 12:27

hannes-steffenhagen-diffblue approved these changes Jan 29, 2021

View reviewed changes

chrisr-diffblue approved these changes Jan 29, 2021

View reviewed changes

tautschnig force-pushed the forall-operands branch from e01fcff to 08bc470 Compare February 12, 2021 00:03

kroening reviewed Feb 17, 2021

View reviewed changes

tautschnig force-pushed the forall-operands branch from 08bc470 to 5ad94ce Compare March 3, 2021 07:49

tautschnig force-pushed the forall-operands branch from 5ad94ce to 9d596f0 Compare March 16, 2021 23:52

tautschnig force-pushed the forall-operands branch from 9d596f0 to 3e59f2e Compare May 7, 2021 06:21

tautschnig force-pushed the forall-operands branch from 3e59f2e to 680354a Compare August 13, 2021 11:20

tautschnig force-pushed the forall-operands branch from 680354a to 4e2f85c Compare October 27, 2022 09:59

tautschnig requested review from thomasspriggs, NlightNFotis, TGWDB and chris-ryder as code owners October 27, 2022 09:59

tautschnig force-pushed the forall-operands branch from 4e2f85c to 139f893 Compare October 27, 2022 11:29

TGWDB approved these changes Oct 28, 2022

View reviewed changes

tautschnig force-pushed the forall-operands branch from 139f893 to 011bc44 Compare November 14, 2022 11:41

NlightNFotis approved these changes Nov 14, 2022

View reviewed changes

tautschnig force-pushed the forall-operands branch from 011bc44 to e4aaa48 Compare November 20, 2022 21:39

tautschnig force-pushed the forall-operands branch from e4aaa48 to 145feb0 Compare December 1, 2022 21:11

tautschnig requested a review from remi-delmas-3000 as a code owner December 1, 2022 21:11

tautschnig assigned tautschnig and unassigned kroening Dec 1, 2022

Replace some uses of forall_operands by ranged-for

7cb1f5b

In some (but not all!) cases of forall_operands we do not actually need the iterator.

tautschnig force-pushed the forall-operands branch from 145feb0 to 7cb1f5b Compare December 1, 2022 21:20

tautschnig merged commit 837cd1b into diffblue:develop Dec 1, 2022

tautschnig deleted the forall-operands branch December 1, 2022 22:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Replace some uses of forall_operands by ranged-for #5790

Replace some uses of forall_operands by ranged-for #5790

Uh oh!

tautschnig commented Jan 26, 2021

Uh oh!

codecov bot commented Jan 26, 2021 •

edited

Loading

Uh oh!

peterschrammel left a comment

Uh oh!

peterschrammel Jan 26, 2021

Uh oh!

martin-cs left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hannes-steffenhagen-diffblue left a comment

Uh oh!

hannes-steffenhagen-diffblue Jan 28, 2021

Uh oh!

kroening Feb 17, 2021

Uh oh!

tautschnig Feb 18, 2021

Uh oh!

Uh oh!

Replace some uses of forall_operands by ranged-for #5790

Replace some uses of forall_operands by ranged-for #5790

Uh oh!

Conversation

tautschnig commented Jan 26, 2021

Uh oh!

codecov bot commented Jan 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

peterschrammel left a comment

Choose a reason for hiding this comment

Uh oh!

peterschrammel Jan 26, 2021

Choose a reason for hiding this comment

Uh oh!

martin-cs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hannes-steffenhagen-diffblue left a comment

Choose a reason for hiding this comment

Uh oh!

hannes-steffenhagen-diffblue Jan 28, 2021

Choose a reason for hiding this comment

Uh oh!

kroening Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

tautschnig Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Jan 26, 2021 •

edited

Loading