Cleanup use of empty strings #4041

tautschnig · 2019-02-02T22:23:06Z

Please review commit-by-commit. The common theme to this PR is a review of all uses of "".

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.

allredj

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

allredj

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

owen-mc-diffblue · 2019-02-15T05:46:07Z

The test failure appears to be clang-format complaining about one line

allredj

⚠️
This PR failed Diffblue compatibility checks (cbmc commit: 385b7f8).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/105262938
Status will be re-evaluated on next push.
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 compatibility was already broken by an earlier merge.

martin-cs

I believe this patch does what it claims to. I think it may even improve performance very slightly but I have to know -- why did you decide to do this?

tautschnig · 2019-03-21T21:03:29Z

why did you decide to do this?

In some prior PR I had something like symbol_exprt("", ...) and someone reminded me that using irep_idt() would be more efficient. Fixing that one instance was easy, but I don't really like doing half a job...

kroening · 2019-04-30T08:21:32Z

src/goto-analyzer/taint_analysis.h

@@ -22,6 +22,6 @@ bool taint_analysis(
  const std::string &taint_file_name,
  message_handlert &,
  bool show_full,
-  const std::string &json_file_name);
+  const std::string &json_output_file_name = "");



How about actually using optionalt?

kroening · 2019-04-30T08:28:47Z

On Use s.empty() instead of s == "":
This may be a case of premature optimisation. Note that the implementation of == on std::string and char * is as follows:

    size_t __rhs_len = _Traits::length(__rhs);
    if (__rhs_len != __lhs.size()) return false;
    return __lhs.compare(0, _String::npos, __rhs, __rhs_len) == 0;

_Traits::length("") will be a compile-time zero; so there is likely no performance benefit. We would need to be convinced that .empty() is indeed better readable than !=""

kroening · 2019-04-30T08:37:00Z

There are a couple of cases where one might obtain better clarity by using optionalt instead of the empty-string as indicator of "no value", but that's beyond this PR.

config.main is now optionalt<string> [blocks: #4041]

We previously passed in empty strings. Make explicit that the argument is not required.

The use of "" requires constructing a std::string or dstringt, and then a string comparison. empty() is just an integer comparison (both for std::string an dstringt).

Both std::string and dstringt are default-initialised to an empty string.

The use of "" requires constructing a std::string or dstringt, and then a string comparison. empty() is just an integer comparison (both for std::string an dstringt).

Constructing a dstringt from a C string requires a string table lookup, while the default constructor for a dstringt just zero-initialises an unsigned.

This avoids string table lookups.

Appending "" to a string has no effect.

This avoids constructing an object when we can just use an integer comparison.

This is follow-up of #4041. It avoids the use of an empty string as indicator of 'no value'.

tautschnig added the cleanup label Feb 2, 2019

tautschnig requested review from allredj, cesaro, chrisr-diffblue, cristina-david, kroening, martin-cs, NathanJPhillips, peterschrammel, pkesseli, romainbrenguier, smowton and thk123 as code owners February 2, 2019 22:23

tautschnig force-pushed the empty-string branch from c851c47 to 8add28f Compare February 2, 2019 23:11

allredj reviewed Feb 3, 2019

View reviewed changes

smowton approved these changes Feb 4, 2019

View reviewed changes

tautschnig force-pushed the empty-string branch from 8add28f to 83324e4 Compare February 4, 2019 13:17

tautschnig assigned kroening, peterschrammel and chrisr-diffblue Feb 4, 2019

tautschnig force-pushed the empty-string branch from 83324e4 to 465e0cc Compare February 5, 2019 11:32

allredj reviewed Feb 5, 2019

View reviewed changes

peterschrammel approved these changes Feb 7, 2019

View reviewed changes

peterschrammel unassigned peterschrammel and chrisr-diffblue Feb 7, 2019

tautschnig force-pushed the empty-string branch from 465e0cc to a7364e4 Compare February 7, 2019 18:06

tautschnig mentioned this pull request Feb 13, 2019

Generic tests: check for matching tag, not completely matching type #4172

Merged

romainbrenguier approved these changes Feb 15, 2019

View reviewed changes

tautschnig force-pushed the empty-string branch from a7364e4 to 68f3400 Compare February 15, 2019 07:31

allredj reviewed Mar 21, 2019

View reviewed changes

martin-cs approved these changes Mar 21, 2019

View reviewed changes

kroening reviewed Apr 30, 2019

View reviewed changes

kroening approved these changes Apr 30, 2019

View reviewed changes

kroening mentioned this pull request Apr 30, 2019

config.main is now optionalt<string> [blocks: #4041] #4591

Merged

3 tasks

tautschnig assigned tautschnig and unassigned kroening Apr 30, 2019

tautschnig changed the title ~~Cleanup use of empty strings~~ Cleanup use of empty strings [depends-on: #4591] Apr 30, 2019

tautschnig added the dependent - do not merge label Apr 30, 2019

tautschnig added a commit that referenced this pull request Apr 30, 2019

Merge pull request #4591 from diffblue/config_main_optional

3d94a62

config.main is now optionalt<string> [blocks: #4041]

tautschnig added 9 commits April 30, 2019 22:28

taint_analysis: make last argument optional

03123a9

We previously passed in empty strings. Make explicit that the argument is not required.

Use s.empty() instead of s == ""

ec56872

The use of "" requires constructing a std::string or dstringt, and then a string comparison. empty() is just an integer comparison (both for std::string an dstringt).

Remove redundant string initialisation

c1baa03

Both std::string and dstringt are default-initialised to an empty string.

Use !s.empty() instead of s != ""

0210503

The use of "" requires constructing a std::string or dstringt, and then a string comparison. empty() is just an integer comparison (both for std::string an dstringt).

Use irep_idt() instead of ""

8a277ef

Constructing a dstringt from a C string requires a string table lookup, while the default constructor for a dstringt just zero-initialises an unsigned.

The second argument to java_type_from_string is optional

672e711

Use s.clear() instead of s=""

7e5b60f

This avoids string table lookups.

Remove unnecessary uses of ""

29a9ce8

Appending "" to a string has no effect.

Use s.empty() or !s.empty() instead of comparison to irep_idt()

13b3e4f

This avoids constructing an object when we can just use an integer comparison.

tautschnig force-pushed the empty-string branch from 385b7f8 to 13b3e4f Compare April 30, 2019 22:40

tautschnig changed the title ~~Cleanup use of empty strings [depends-on: #4591]~~ Cleanup use of empty strings Apr 30, 2019

tautschnig removed the dependent - do not merge label Apr 30, 2019

tautschnig merged commit d6eae52 into diffblue:develop Apr 30, 2019

tautschnig deleted the empty-string branch April 30, 2019 23:54

kroening pushed a commit that referenced this pull request May 1, 2019

use optional for json file name

e84180e

This is follow-up of #4041. It avoids the use of an empty string as indicator of 'no value'.

kroening mentioned this pull request May 1, 2019

use optional for json file name #4597

Merged

3 tasks

kroening pushed a commit that referenced this pull request May 1, 2019

use optional for json file name

66b5abb

This is follow-up of #4041. It avoids the use of an empty string as indicator of 'no value'.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Cleanup use of empty strings #4041

Cleanup use of empty strings #4041

Uh oh!

tautschnig commented Feb 2, 2019

Uh oh!

allredj left a comment

Uh oh!

allredj left a comment

Uh oh!

owen-mc-diffblue commented Feb 15, 2019

Uh oh!

allredj left a comment

Uh oh!

martin-cs left a comment

Uh oh!

tautschnig commented Mar 21, 2019

Uh oh!

kroening Apr 30, 2019

Uh oh!

kroening commented Apr 30, 2019

Uh oh!

kroening commented Apr 30, 2019

Uh oh!

Uh oh!

Cleanup use of empty strings #4041

Cleanup use of empty strings #4041

Uh oh!

Conversation

tautschnig commented Feb 2, 2019

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!

owen-mc-diffblue commented Feb 15, 2019

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

martin-cs left a comment

Choose a reason for hiding this comment

Uh oh!

tautschnig commented Mar 21, 2019

Uh oh!

kroening Apr 30, 2019

Choose a reason for hiding this comment

Uh oh!

kroening commented Apr 30, 2019

Uh oh!

kroening commented Apr 30, 2019

Uh oh!

Uh oh!