Cleanup of operands() accesses #23

tautschnig · 2016-03-13T12:30:45Z

Although USE_LIST is gone as of r3530, there are a number of cases where iterators are preferable over indexed access to elements.
This is a duplicate of #9 after restoring all links to the SVN.

thk123

This is an old PR so may be too distant for you to remember it's purpose!I've had a look through and changes all look like they don't change behaviour just improve type correctness and better for loops.

Main changes are consistency with prefix vs. post fix increamentors and a couple of places that can use range based for.

This also needs to be rebased - which may be more effort than it is worth.

thk123 · 2016-12-22T17:07:05Z

src/cpp/cpp_typecheck_expr.cpp

@@ -2578,25 +2578,26 @@ void cpp_typecheckt::typecheck_function_call_arguments(
    }
  }

-  for(std::size_t i=0; i<parameters.size(); i++)
+  exprt::operandst::iterator arg_it=expr.arguments().begin();
+  for(std::size_t i=0; i<parameters.size(); i++, ++arg_it)


Use either prefix or postfix to increment these.

thk123 · 2016-12-22T17:11:40Z

src/cpp/cpp_typecheck_resolve.cpp

@@ -2634,10 +2635,12 @@ void cpp_typecheck_resolvet::resolve_with_arguments(
  const cpp_typecheck_fargst &fargs)
 {
  // not clear what this is good for
-  for(std::size_t i=0; i<fargs.operands.size(); i++)
+  for(exprt::operandst::const_iterator it=fargs.operands.begin();


Can use a ranged for here

thk123 · 2016-12-22T17:15:20Z

src/util/expr_util.cpp


-  for(std::size_t i=1; i<operands.size(); i++)
+  for(exprt::operandst::const_iterator


Range based for.

thk123 · 2016-12-22T17:16:16Z

src/util/std_code.h

@@ -282,7 +282,7 @@ class code_assumet:public codet
  inline code_assumet():codet(ID_assume)
  {
    // will change to resize(1) in the future
-    operands().reserve(1);
+    reserve_operands(1);


Is the comment above still relevant/correct? Is reserve_operands what resize ended up being called?

tautschnig · 2016-12-23T15:57:00Z

I have now refreshed/rebased this pull request, and have taken into account all the comments as far as applicable.

peterschrammel · 2017-01-11T18:35:51Z

@thk123, can you please check whether your comments have been taken into account?

thk123 · 2017-01-12T10:48:22Z

@peterschrammel I think these (1, 3) comments are still outstanding. Are there reasons for not addressing them @tautschnig?

tautschnig · 2017-01-18T15:47:32Z

I have addressed the remaining comments, which induced some further cleanup.

thk123

Last few changes - apologies about wrongly saying should use op0() rather than front(), should have checked was an exprt not an operandst - I have removed this comment.

thk123 · 2017-01-19T09:50:17Z

src/cpp/cpp_typecheck_expr.cpp


        exprt temporary;
-        new_temporary(expr.arguments()[i].source_location(),
-                      parameters[i].type().subtype(),
+        new_temporary(arg_it->source_location(),


The parameter should be on the next line. It might be worth rebasing again so you get the lint output from just modified lines.

thk123 · 2017-01-19T09:51:31Z

src/cpp/cpp_typecheck_fargs.cpp

@@ -109,7 +109,8 @@ bool cpp_typecheck_fargst::match(
      return false;
  }

-  for(std::size_t i=0; i<ops.size(); i++)
+  exprt::operandst::iterator it=ops.begin();
+  for(std::size_t i=0; i<ops.size(); i++, ++it)


Still mixing post fix and prefix incrementing. Maybe this isn't really a problem (I think it looks weird but it isn't in the coding guidelines) but I thought I'd see this being picked up in other PR reviews.

thk123 · 2017-01-19T09:58:11Z

src/util/std_code.h

    return op0();
  }

  inline bool has_return_value() const
  {
-    return operands().size()==1;
+    if(operands().empty()) return false; // backwards compatibility


False on the new line.

if(operands().empty()) return false; // backwards compatibility

Use iterators instead of indexed access. This may re-enable std::list support (for exprt::operandst) if taken further.

tautschnig · 2017-01-20T16:42:25Z

Thanks a lot for the careful scrutiny! I believe I have addressed the issues, but looking at the cpplint run yields some rather unrelated problems. @thk123 Could you please take a look at the travis output?

thk123 · 2017-01-23T14:09:20Z

@tautschnig You seem to be based off a commit that is before the script that just checks the modified lines, if you rebase I imagine they will go away (or at least the only ones left should be legitimate errors). It should also fix the python crash at the end.

…ession-broken Fixed taint configuration

abstraction: put all functions into one namespace/class

tautschnig force-pushed the operandst-list-support branch from 9c5a1ae to f907248 Compare March 21, 2016 19:21

tautschnig force-pushed the operandst-list-support branch from f907248 to bad02a1 Compare April 3, 2016 09:51

kroening self-assigned this Apr 18, 2016

tautschnig force-pushed the operandst-list-support branch from bad02a1 to bd098eb Compare May 18, 2016 17:13

tautschnig added the enhancement label May 29, 2016

tautschnig force-pushed the operandst-list-support branch from bd098eb to a4145aa Compare May 30, 2016 11:44

tautschnig force-pushed the operandst-list-support branch from a4145aa to b4e83a7 Compare June 12, 2016 13:13

tautschnig force-pushed the operandst-list-support branch from b4e83a7 to 5498001 Compare June 21, 2016 07:12

tautschnig force-pushed the operandst-list-support branch from 5498001 to a01e3a1 Compare November 5, 2016 22:25

thk123 suggested changes Dec 22, 2016

View reviewed changes

tautschnig force-pushed the operandst-list-support branch from a01e3a1 to e1b120b Compare December 23, 2016 15:56

tautschnig force-pushed the operandst-list-support branch from e1b120b to e76a3a3 Compare January 18, 2017 15:47

tautschnig force-pushed the operandst-list-support branch from e76a3a3 to 383a9de Compare January 18, 2017 15:51

thk123 suggested changes Jan 19, 2017

View reviewed changes

tautschnig added 2 commits January 20, 2017 07:09

Cleanup of operands() accesses

c1304cd

Use iterators instead of indexed access. This may re-enable std::list support (for exprt::operandst) if taken further.

Consistently resize instead of just reserving in codet classes

e527f64

tautschnig force-pushed the operandst-list-support branch from 383a9de to e527f64 Compare January 20, 2017 15:09

kroening merged commit 97e038f into diffblue:master Jan 23, 2017

tautschnig deleted the operandst-list-support branch January 25, 2017 13:29

smowton added a commit to smowton/cbmc that referenced this pull request May 9, 2018

Merge pull request diffblue#23 from NathanJPhillips/bugfix/taint-regr…

c5f59e8

…ession-broken Fixed taint configuration

zlfben pushed a commit to zlfben/cbmc that referenced this pull request May 13, 2021

Merge pull request diffblue#23 from lefanz-amazon/lefan-abstraction

1851e9a

abstraction: put all functions into one namespace/class

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Cleanup of operands() accesses #23

Cleanup of operands() accesses #23

Uh oh!

tautschnig commented Mar 13, 2016

Uh oh!

thk123 left a comment

Uh oh!

thk123 Dec 22, 2016

Uh oh!

thk123 Dec 22, 2016

Uh oh!

thk123 Dec 22, 2016

Uh oh!

thk123 Dec 22, 2016

Uh oh!

tautschnig commented Dec 23, 2016

Uh oh!

peterschrammel commented Jan 11, 2017

Uh oh!

thk123 commented Jan 12, 2017

Uh oh!

tautschnig commented Jan 18, 2017

Uh oh!

thk123 left a comment •

edited

Loading

Uh oh!

thk123 Jan 19, 2017

Uh oh!

thk123 Jan 19, 2017

Uh oh!

thk123 Jan 19, 2017

Uh oh!

tautschnig commented Jan 20, 2017

Uh oh!

thk123 commented Jan 23, 2017

Uh oh!

Uh oh!


		for(std::size_t i=1; i<operands.size(); i++)
		for(exprt::operandst::const_iterator

Cleanup of operands() accesses #23

Cleanup of operands() accesses #23

Uh oh!

Conversation

tautschnig commented Mar 13, 2016

Uh oh!

thk123 left a comment

Choose a reason for hiding this comment

Uh oh!

thk123 Dec 22, 2016

Choose a reason for hiding this comment

Uh oh!

thk123 Dec 22, 2016

Choose a reason for hiding this comment

Uh oh!

thk123 Dec 22, 2016

Choose a reason for hiding this comment

Uh oh!

thk123 Dec 22, 2016

Choose a reason for hiding this comment

Uh oh!

tautschnig commented Dec 23, 2016

Uh oh!

peterschrammel commented Jan 11, 2017

Uh oh!

thk123 commented Jan 12, 2017

Uh oh!

tautschnig commented Jan 18, 2017

Uh oh!

thk123 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thk123 Jan 19, 2017

Choose a reason for hiding this comment

Uh oh!

thk123 Jan 19, 2017

Choose a reason for hiding this comment

Uh oh!

thk123 Jan 19, 2017

Choose a reason for hiding this comment

Uh oh!

tautschnig commented Jan 20, 2017

Uh oh!

thk123 commented Jan 23, 2017

Uh oh!

Uh oh!

thk123 left a comment •

edited

Loading