JSON equality operator #4912

LAJW · 2019-07-15T15:53:58Z

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.
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).
My PR is restricted to a single feature or bugfix.

thomasspriggs

Looks useful for writing unit tests.

thomasspriggs · 2019-07-15T16:08:46Z

src/util/json.cpp

+    const auto &right_object = static_cast<const json_objectt &>(right);
+    const auto yes = [](const std::pair<std::string, jsont> &) { return true; };
+    const size_t left_size = narrow<size_t>(
+      std::count_if(left_object.begin(), left_object.end(), yes));


❔ Can this be done using std::count, so that you don't need to define yes?

std::count counts occurrences of a specified element, so no.

smowton · 2019-07-16T08:03:27Z

src/util/json.cpp

+    const auto &right_object = static_cast<const json_objectt &>(right);
+    const auto yes = [](const std::pair<std::string, jsont> &) { return true; };
+    const size_t left_size = narrow<size_t>(
+      std::count_if(left_object.begin(), left_object.end(), yes));


std::distance(left_object.begin(), left_object.end())

Or since object is std::map, just use .size(). At present since it is map you could also use zip and safely assume the keys occur in the same order, if guarded by a static-assert checking that typeof(object) really is an (ordered) map.

json_objectt is not a map, it just has some map-like methods exposed. size sadly is not one of them.
zip lives in test-gen AFAIK.
I don't want to rely on the order of elements in the map. We've been shifting towards using unordered_map in many places, and JSON looks like it could be one of them.

json_objectt is not, but its member object is. zip lives in https://github.com/diffblue/cbmc/blob/develop/src/util/range.h

Sure, but if you guard with an appropriate static_assert (ideally there would be some type trait is_ordered) then you could be sure this code will be changed as/when we do that

object is a protected property.

There isn't an is_ordered trait. Being "ordered" is a runtime property.

If I didn't have to worry about future proofing - zip is unnecessary - std::equal(b1, e1, b2) is the tool for this job.

At least expose a size property on json_objectt rather than hacking around it with the existing interface. The ordered vs. unordered thing I'll pass on.

allredj

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

codecov-io · 2019-07-16T10:56:49Z

Codecov Report

Merging #4912 into develop will decrease coverage by 0.06%.
The diff coverage is 98.43%.

@@             Coverage Diff             @@
##           develop    #4912      +/-   ##
===========================================
- Coverage    69.19%   69.12%   -0.07%     
===========================================
  Files         1307     1300       -7     
  Lines       107955   107057     -898     
===========================================
- Hits         74700    74007     -693     
+ Misses       33255    33050     -205

Impacted Files	Coverage Δ
src/util/json.h	`95.52% <ø> (ø)`	⬆️
unit/json/json_parser.cpp	`100% <100%> (ø)`	⬆️
src/util/json.cpp	`85.93% <93.33%> (+2.26%)`	⬆️
src/statement-list/statement_list_typecheck.h	`0% <0%> (-100%)`	⬇️
src/statement-list/statement_list_typecheck.cpp	`0% <0%> (-65.1%)`	⬇️
src/statement-list/statement_list_language.cpp	`44% <0%> (-24%)`	⬇️
src/statement-list/parser.y	`75.98% <0%> (-13.73%)`	⬇️
...statement-list/converters/convert_string_value.cpp	`72.72% <0%> (-7.28%)`	⬇️
src/util/simplify_expr.cpp	`81.64% <0%> (-5.65%)`	⬇️
src/statement-list/scanner.l	`72.97% <0%> (-3.47%)`	⬇️
... and 46 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e356c2...c70cbab. Read the comment docs.

tautschnig · 2019-07-16T11:21:52Z

src/util/json.cpp

 \*******************************************************************/

 #include "json.h"
+#include <util/narrow.h>


For consistency: #include "narrow.h"

tautschnig · 2019-07-16T11:24:42Z

src/util/json.cpp

+  case jsont::kindt::J_NUMBER:
+    return left.value == right.value;
+  case jsont::kindt::J_STRING:
+    return left.value == right.value;


Overall design question: shouldn't we do this more in a visitor-style where each call deriving from jsont defines each own operator== and the switch-case here would only do the downcasts and then invoke the more specific equality operators?

Yes, except jsont much like irept doesn't allow any virtual functions. Therefore the visitor pattern is not applicable.

Of course, hence me saying "visitor-style." But also my comment was intended as a thought, not any kind of hard requirement.

tautschnig · 2019-07-16T11:29:02Z

src/util/json.cpp

+    const size_t left_size =
+      narrow<size_t>(std::distance(left_object.begin(), left_object.end()));
+    const size_t right_size =
+      narrow<size_t>(std::distance(right_object.begin(), right_object.end()));


Why do those conversions when afterwards you're only going to use the values for comparing them to each other? I'd go with const auto left_size = std::distance ...

tautschnig · 2019-07-16T11:30:37Z

src/util/json.cpp

+      left_object.begin(),
+      left_object.end(),
+      [&](const std::pair<std::string, jsont> &pair) {
+        return right_object[pair.first] == pair.second;


That's an unnecessary lookup when instead you could just use two iterators over the two containers.

Yeah, but if someone decides to swap std::map for std::unordered_map, it could be difficult to debug, so this is more stable/doesn't rely on any invariants.

I do wonder which containers would exhibit differences in strict equality testing? But I suppose it is conceivable that two std::unordered_map had their elements constructed in a different order, yet should compare equal. So, yes, keep it as is.

Personally I still favour

// This relies on ordered maps: static_assert(std::is_same<typeof(jsont::object), std::map<std::string, jsont>>::value)

tautschnig · 2019-07-16T11:31:56Z

unit/json/json_parser.cpp

  }
 }
+
+TEST_CASE("json equality", "[core][test-gen-util][state][type]")


I actually don't know what those [...][...] parts are good for at all, but this particular entry ("test-gen-util") strikes me as information of no value in this repository.

tautschnig · 2019-07-16T11:35:14Z

scripts/cpplint.py

-    if (not Match(r'^[\s}]*[{.;,)<>\]:]', trailing_text)
-        and not _IsType(clean_lines, nesting_state, leading_text)):
-      error(filename, linenum, 'whitespace/braces', 5,
-            'Missing space before {')


Maybe removing this is the right thing to do, but then we should remove all the code computing leading_text, trailing_text above. Or maybe you should just use NOLINTNEXTLINE(whitespace/braces) in the test.

clang-format already checks for positions and spacing of curly braces and parentheses. The problem stems from the fact this isn't aware of initializer lists/uniform initialization.

std::vector<std::pair<int, int>>{{1, 2}, {3, 4}, {5, 6}};

In the days of C++98, the above wasn't a valid syntax. Curly braces would appear only after if /for statements, as block scopes or surround function bodies. The linter expects this to still be the case.

Ok, in which case the linter should be cleaned properly so as not to leave in place unused code.

allredj

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

LAJW · 2019-07-16T16:18:17Z

Done!

Exposed jsont::object::size as json_objectt::size
Used that instead of std::distance
Removed dead linter code

allredj

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

thomasspriggs · 2019-07-16T18:10:35Z

src/util/json.cpp

+    return left.value == right.value;
+  case jsont::kindt::J_ARRAY:
+  {
+    const auto &left_array = static_cast<const json_arrayt &>(left);


ℹ️ There is a function called to_json_array at the bottom of this file, which you could re-use instead of adding your own static casts.

static_cast is a feature of C++, and not "my" cast. to_json_array checks for kind, which in this case is already checked. We'd be doing extra work, so no.

Because it'd be useful to be able to compare whole JSON objects (for instance in unit tests)

Braces are already checked by clang-format, and this rule doesn't allow me to use initializer list constructors/uniform initialization in unit tests

thk123

Code owner review of the cpplint change only (though the other changes lgtm)

allredj

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

This is to avoid clang-format silently aborting when encountering Java files, as happened in diffblue#4912. The configuration for Java likely needs tweaking to avoid mass-reformatting, but we might do so in an incremental fashion.

LAJW requested review from thk123 and thomasspriggs July 15, 2019 15:53

LAJW requested review from kroening, peterschrammel, pkesseli, smowton and tautschnig as code owners July 15, 2019 15:53

thomasspriggs approved these changes Jul 15, 2019

View reviewed changes

smowton reviewed Jul 16, 2019

View reviewed changes

allredj reviewed Jul 16, 2019

View reviewed changes

tautschnig reviewed Jul 16, 2019

View reviewed changes

tautschnig assigned LAJW Jul 16, 2019

allredj reviewed Jul 16, 2019

View reviewed changes

Add size property to json_objectt

6ed1c12

smowton approved these changes Jul 16, 2019

View reviewed changes

allredj reviewed Jul 16, 2019

View reviewed changes

thomasspriggs reviewed Jul 16, 2019

View reviewed changes

Lukasz A.J. Wrona added 2 commits July 17, 2019 13:51

JSON equality operator and tests

799768e

Because it'd be useful to be able to compare whole JSON objects (for instance in unit tests)

Remove linter braces rule

c70cbab

Braces are already checked by clang-format, and this rule doesn't allow me to use initializer list constructors/uniform initialization in unit tests

thk123 approved these changes Jul 17, 2019

View reviewed changes

allredj reviewed Jul 17, 2019

View reviewed changes

LAJW merged commit f6ea83c into diffblue:develop Jul 17, 2019

karkhaz mentioned this pull request Jul 17, 2019

Fix #include order in util/json.cpp #4922

Merged

tautschnig mentioned this pull request Jul 18, 2019

Enable clang-format for Java #4926

Merged

2 tasks

LAJW deleted the lajw/json-equality branch October 21, 2019 09:42

JSON equality operator #4912

JSON equality operator #4912

Uh oh!

Conversation

LAJW commented Jul 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomasspriggs 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!

LAJW Jul 16, 2019 • 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!

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!

smowton Jul 16, 2019 • 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!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Jul 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

smowton Jul 16, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LAJW Jul 16, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

LAJW commented Jul 16, 2019

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

LAJW commented Jul 15, 2019 •

edited

Loading

LAJW Jul 16, 2019 •

edited

Loading

smowton Jul 16, 2019 •

edited

Loading

codecov-io commented Jul 16, 2019 •

edited

Loading

smowton Jul 16, 2019 •

edited

Loading

LAJW Jul 16, 2019 •

edited

Loading