Cleanup of operands() accesses#23
Conversation
tautschnig
force-pushed
the
operandst-list-support
branch
from
9c5a1ae to
f907248
Compare
tautschnig
force-pushed
the
operandst-list-support
branch
from
f907248 to
bad02a1
Compare
tautschnig
force-pushed
the
operandst-list-support
branch
from
bad02a1 to
bd098eb
Compare
tautschnig
force-pushed
the
operandst-list-support
branch
from
bd098eb to
a4145aa
Compare
tautschnig
force-pushed
the
operandst-list-support
branch
from
a4145aa to
b4e83a7
Compare
tautschnig
force-pushed
the
operandst-list-support
branch
from
b4e83a7 to
5498001
Compare
tautschnig
force-pushed
the
operandst-list-support
branch
from
5498001 to
a01e3a1
Compare
thk123
left a comment
thk123
left a comment
There was a problem hiding this comment.
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.
src/cpp/cpp_typecheck_expr.cpp
Outdated
There was a problem hiding this comment.
Use either prefix or postfix to increment these.
src/cpp/cpp_typecheck_resolve.cpp
Outdated
src/util/expr_util.cpp
Outdated
src/util/std_code.h
Outdated
There was a problem hiding this comment.
Is the comment above still relevant/correct? Is reserve_operands what resize ended up being called?
tautschnig
force-pushed
the
operandst-list-support
branch
from
a01e3a1 to
e1b120b
Compare
|
I have now refreshed/rebased this pull request, and have taken into account all the comments as far as applicable. |
|
@thk123, can you please check whether your comments have been taken into account? |
|
@peterschrammel I think these (1, 3) comments are still outstanding. Are there reasons for not addressing them @tautschnig? |
tautschnig
force-pushed
the
operandst-list-support
branch
from
e1b120b to
e76a3a3
Compare
|
I have addressed the remaining comments, which induced some further cleanup. |
tautschnig
force-pushed
the
operandst-list-support
branch
from
e76a3a3 to
383a9de
Compare
src/cpp/cpp_typecheck_expr.cpp
Outdated
There was a problem hiding this comment.
The parameter should be on the next line. It might be worth rebasing again so you get the lint output from just modified lines.
src/cpp/cpp_typecheck_fargs.cpp
Outdated
There was a problem hiding this comment.
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.
src/util/std_code.h
Outdated
There was a problem hiding this comment.
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
force-pushed
the
operandst-list-support
branch
from
383a9de to
e527f64
Compare
|
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? |
|
@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
5 participants
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.