Avoid shadowing of variables [blocks: #2310] #3607

tautschnig · 2018-12-20T13:55:20Z

Please review commit by commit - each of them solves a variable shadowing problem in some subset of classes/files.

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.

smowton · 2018-12-20T14:40:20Z

src/cpp/cpp_typecheck_virtual_table.cpp

      id2string(vt_symb_type.name) + "@"+ id2string(symbol.name);
    vt_symb_var.base_name=
      id2string(vt_symb_type.base_name) + "@" + id2string(symbol.base_name);
-    vt_symb_var.mode=ID_cpp;


Unrelated change?

Hmm, yes, I'll remove it.

Hmm, looking at my commit message that's actually explained:

Do not shadow "mode" in cpp front-end Also pick up a previously set mode from a symbol rather than hard-coding ID_cpp where possible. The symbol's mode could be ID_C or ID_cpp, depending on the linkage specification.

I'm a little uncomfortable with either the commit message, or the commit - the headline of the message 'Do not shadow "mode" in cpp front-end' sounds like a simple refactoring, yet the commit also has a functional change in there too (which admittedly is mentioned in the body of the message) - i.e. the handling of previously set modes. Might it be better to either split that change out in to a separate commit, or change the wording of this commit so that the headline is more along the lines of 'Improve handling of "mode" in cpp frontend' ?

I have taken the route of amending the commit message as suggested.

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: cceb8f4).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/95450432

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: a94836e).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/95509779

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: e32dc1a).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/95526623

src/goto-cc/gcc_cmdline.h

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: 2208e45).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/95541818

allredj

🚫
This PR failed Diffblue compatibility checks (cbmc commit: d077960).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/95543217
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.

Common spurious failures:

the cbmc commit has disappeared in the mean time (e.g. in a force-push)
the author is not in the list of contributors (e.g. first-time contributors).

The incompatibility may have been introduced by an earlier PR. In that case merging this
PR should be avoided unless it fixes the current incompatibility.

Avoid shadowing in directory jbmc/unit [blocks: #3607]

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: 2da216c).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/96499375

thk123 · 2019-01-08T11:31:23Z

src/solvers/refinement/string_refinement.cpp

  {
    dependencies.clean_cache();
-    const decision_proceduret::resultt res=supert::dec_solve();
+    res = supert::dec_solve();


I'm not a big fan of this change - makes it hard to see that this is constant throughout the loop and not dependent on it's outside loop state. A nice fix would of course be to introduce some functions or something, but perhaps an easier immediate fix for the shadowing would be call this loop_res and the other one initial_res

Renaming done as suggested (using initial_result, refined_result).

thk123 · 2019-01-08T11:39:15Z

src/solvers/refinement/string_constraint_generator_comparison.cpp

 {
  PRECONDITION(f.arguments().size() == 1);
-  string_constraintst constraints;
+  string_constraintst hash_constraints;


⛏️ I'd probably rename the member variable to global_constraints or all_constraints

Please forgive me for not intending to act on this one: from reading the code I got the impression that the two cases touched here affect methods that should likely be deprecated or removed. @romainbrenguier Any insights maybe?

No problem - all of these comments are optional, I'm not overly familiar with the code being modified here anyway.

Yes hash code and intern should be implemented via models rather than internally in the solver (they are very language dependent), so this should disappear.

thk123 · 2019-01-08T11:40:25Z

src/solvers/refinement/string_constraint_generator_float.cpp

-    conversion_factor_table.copy_to_operands(constant_float(f, float_spec));
-  for(const auto &f : two_power_e_over_ten_power_d_table)
-    conversion_factor_table.copy_to_operands(constant_float(f, float_spec));
+  for(const auto &neg : two_power_e_over_ten_power_d_table_negatives)


⛏️ Don't use abbreviations negative, position - though obviously a massive improvement on what was there before.
⛏️ Rename the param f to float_expr?

Done, and a separate commit doing the renaming of f is being prepared.

There is a class member of the same name, use "symbols_in_constraint" instead.

There is a class member named "result", rename the local variable to "eval_result."

Rename both instances, the first to "initial_result" and the second to "refined_result."

Rename the first use to "initial_instances"

There is no need to pass a class member as an argument.

Rename the method-local ones "intern_constraints" and "hash_constraints."

Rename container elements to "positive" and "negative."

A single-character name is not very descriptive, longer symbol names bear no runtime overhead.

allredj

🚫
This PR failed Diffblue compatibility checks (cbmc commit: 6e271c3).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/96602029
Status will be re-evaluated on next push.
Please contact @peterschrammel, @thk123, or @allredj for support.

Common spurious failures:

the cbmc commit has disappeared in the mean time (e.g. in a force-push)
the author is not in the list of contributors (e.g. first-time contributors).

The incompatibility may have been introduced by an earlier PR. In that case merging this
PR should be avoided unless it fixes the current incompatibility.

allredj

✔️
Passed Diffblue compatibility checks (cbmc commit: adde39e).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/96604752

tautschnig added cleanup blocker labels Dec 20, 2018

tautschnig requested review from chrisr-diffblue, kroening, martin-cs, peterschrammel, pkesseli, smowton and thk123 as code owners December 20, 2018 13:55

smowton reviewed Dec 20, 2018

View reviewed changes

smowton approved these changes Dec 20, 2018

View reviewed changes

tautschnig force-pushed the vs-shadow branch from ea510d6 to cceb8f4 Compare December 20, 2018 16:34

tautschnig assigned kroening, peterschrammel, martin-cs and chrisr-diffblue Dec 20, 2018

allredj reviewed Dec 20, 2018

View reviewed changes

tautschnig force-pushed the vs-shadow branch from cceb8f4 to 20d3381 Compare December 21, 2018 07:19

tautschnig requested review from cesaro and cristina-david as code owners December 21, 2018 07:19

tautschnig force-pushed the vs-shadow branch from 20d3381 to a94836e Compare December 21, 2018 09:04

allredj reviewed Dec 21, 2018

View reviewed changes

tautschnig force-pushed the vs-shadow branch from a94836e to e32dc1a Compare December 21, 2018 12:08

allredj reviewed Dec 21, 2018

View reviewed changes

owen-mc-diffblue approved these changes Dec 21, 2018

View reviewed changes

src/goto-cc/gcc_cmdline.h Outdated Show resolved Hide resolved

tautschnig force-pushed the vs-shadow branch from e32dc1a to d077960 Compare December 21, 2018 14:24

tautschnig requested a review from NathanJPhillips as a code owner December 21, 2018 14:24

tautschnig force-pushed the vs-shadow branch from d077960 to 2208e45 Compare December 21, 2018 14:26

allredj reviewed Dec 21, 2018

View reviewed changes

tautschnig force-pushed the vs-shadow branch from 5c8faba to e68bfcc Compare January 4, 2019 12:52

tautschnig changed the title ~~Avoid shadowing of variables [blocks: #2310]~~ Avoid shadowing of variables [depends-on: #3685, blocks: #2310] Jan 5, 2019

tautschnig added dependent - do not merge and removed work in progress labels Jan 5, 2019

tautschnig added a commit that referenced this pull request Jan 7, 2019

Merge pull request #3685 from tautschnig/vs-shadow-jbmc-unit

0b3f43b

Avoid shadowing in directory jbmc/unit [blocks: #3607]

tautschnig changed the title ~~Avoid shadowing of variables [depends-on: #3685, blocks: #2310]~~ Avoid shadowing of variables [blocks: #2310] Jan 7, 2019

tautschnig removed the dependent - do not merge label Jan 7, 2019

tautschnig force-pushed the vs-shadow branch 2 times, most recently from 44d29c8 to 2da216c Compare January 7, 2019 16:04

allredj reviewed Jan 7, 2019

View reviewed changes

tautschnig assigned peterschrammel, martin-cs and romainbrenguier and unassigned tautschnig Jan 7, 2019

thk123 approved these changes Jan 8, 2019

View reviewed changes

tautschnig added 8 commits January 8, 2019 11:57

Do not shadow "symbols"

885fe7d

There is a class member of the same name, use "symbols_in_constraint" instead.

Do not shadow "result"

fbc56c0

There is a class member named "result", rename the local variable to "eval_result."

Do not shadow "res"

2c45dae

Rename both instances, the first to "initial_result" and the second to "refined_result."

Do not shadow "instances"

fbd6b58

Rename the first use to "initial_instances"

Do not shadow "fresh_symbol"

04493f3

There is no need to pass a class member as an argument.

Do not shadow "constraints"

fcbc914

Rename the method-local ones "intern_constraints" and "hash_constraints."

Do not shadow parameter "f"

4c2f940

Rename container elements to "positive" and "negative."

Rename parameter f to float_expr

adde39e

A single-character name is not very descriptive, longer symbol names bear no runtime overhead.

tautschnig force-pushed the vs-shadow branch 2 times, most recently from 6e271c3 to adde39e Compare January 8, 2019 12:18

allredj reviewed Jan 8, 2019

View reviewed changes

romainbrenguier approved these changes Jan 9, 2019

View reviewed changes

tautschnig merged commit b0f2506 into diffblue:develop Jan 9, 2019

tautschnig deleted the vs-shadow branch January 9, 2019 11:39

Avoid shadowing of variables [blocks: #2310] #3607

Avoid shadowing of variables [blocks: #2310] #3607

Uh oh!

Conversation

tautschnig commented Dec 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants