Skip to content

Conversation

@sonodtt
Copy link
Contributor

@sonodtt sonodtt commented Sep 4, 2018

Cleanup of util directory, files j-r (json_irep.cpp withheld awaiting a review of cbmc exceptions)

  1. Cleanup of asserts - replaced with invariants.

  2. Removed all unstructured throw(...) statements, replacing with INVARIANT, PRECONDITION, where appropriate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One invariant I'd change, also please run git-clang-format

ullong_t shift=b.to_ulong();
if(shift>true_size)
throw "shift value out of range";
DATA_INVARIANT(shift<=true_size, "shift value must be in range");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PRECONDITION

@sonodtt sonodtt force-pushed the invariant-cleanup-util_dir-files_j-to-r branch from ebc8391 to 460ff53 Compare September 4, 2018 13:13
@sonodtt
Copy link
Contributor Author

sonodtt commented Sep 4, 2018

@hannes-steffenhagen-diffblue (throws ashes on head ) - I did not push local changes. :(
Done.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, minor ⛏️ aside

ullong_t shift=b.to_ulong();
if(shift>true_size)
throw "shift value out of range";
DATA_INVARIANT(shift <= true_size, "shift value must be in range");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should still be a PRECONDITION

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. It is now that I pushed the local changes. (Doh!)

assert(old_d->ref_count!=0);
PRECONDITION(old_d->ref_count != 0);

#ifdef REFERENCE_COUNTING_DEBUG

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like an odd change, was this done by clang-format?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have undone those indentations and it did not happen again upon a clang-format. (!?!)

@sonodtt sonodtt force-pushed the invariant-cleanup-util_dir-files_j-to-r branch from 460ff53 to 3a915bf Compare September 4, 2018 14:00
Copy link
Collaborator

@tautschnig tautschnig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good except as noted below.

INVARIANT(
!not_found,
"we are assuming that a name exists in the namespace "
"when this function is called");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please include/keep id2string(name) in the error message.

/// Rational Numbers

#include "rational.h"
#include "invariant.h"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maintain lexicographic ordering of includes: move one line up.

Copy link
Contributor Author

@sonodtt sonodtt Sep 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first was the source file's own header. My lack of a spacing line did not make this clear.
Amended to:

#include "rational.h"

#include <algorithm>
#include <ostream>

#include "invariant.h"

#ifndef CPROVER_UTIL_REFERENCE_COUNTING_H
#define CPROVER_UTIL_REFERENCE_COUNTING_H

#include <cassert>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should now be removed.

@sonodtt sonodtt force-pushed the invariant-cleanup-util_dir-files_j-to-r branch from 3a915bf to 8f28c2f Compare September 4, 2018 15:14
Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR failed Diffblue compatibility checks (cbmc commit: 3a915bf).
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).

@sonodtt sonodtt force-pushed the invariant-cleanup-util_dir-files_j-to-r branch from 8f28c2f to 341f993 Compare September 5, 2018 07:46
Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR failed Diffblue compatibility checks (cbmc commit: 341f993).
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).

Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passed Diffblue compatibility checks (cbmc commit: 341f993).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/83828018

@hannes-steffenhagen-diffblue hannes-steffenhagen-diffblue merged commit 55a52a2 into diffblue:develop Sep 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants