Fix narrowing conversions seen by test-gen by LAJW · Pull Request #3900 · diffblue/cbmc

LAJW · 2019-01-23T16:48:32Z

I want to enable conversion warnings in test-gen. Unfortunately, I can't because CBMC uses unchecked conversions in its headers. Because there were only so few, rather than guard CBMC headers, I decided to just fix them.

I'm also including a narrow_cast and narrow (inspired by Guidelines Support Library) with this to mark conversion warnings as such.

Rationale:

Why narrow and narrow_cast and why would you care
Why numeric_cast instead of static_cast if they have the same behaviour
Each commit message has a non-empty body, explaining why the change was made.
Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
My PR is restricted to a single feature or bugfix.
White-space or formatting changes outside the feature-related changed lines are in commits of their own.

tautschnig · 2019-01-23T16:53:22Z

One day I'll actually be getting PRs reviewed and duplicate work will no longer be happening... See #2551, which is factored out from #2310. Anyway, the approach taken here is cleaner. I'll review and will then approve...

hannes-steffenhagen-diffblue

Looks reasonable to me, copyright needs to be sorted out though. Also FWIW I think it'd make more sense to use narrow for all of these cases, I don't think any of them are meant to over/underflow

hannes-steffenhagen-diffblue · 2019-01-23T16:53:12Z

src/util/narrow.h

@@ -0,0 +1,34 @@
+// Copyright 2019 Diffblue Limited. All Rights Reserved.


hannes-steffenhagen-diffblue · 2019-01-23T16:53:35Z

src/util/narrow.h

+/// Alias for static_cast intended to be used for numeric casting
+/// Rationale: Easier to grep than static_cast
+template <typename output_type, typename input_type>
+output_type narrow_cast(input_type value)


If this is derived from Microsoft GSL this might need an appropriate copyright notice to that effect

Not necessary, I wrote it from scratch in test-gen.

hannes-steffenhagen-diffblue · 2019-01-23T16:57:33Z

src/util/narrow.h

+  static_assert(
+    std::is_arithmetic<input_type>::value &&
+      std::is_arithmetic<output_type>::value,
+    "numeric_cast is intended only for numeric conversions");


s/numeric_cast/narrow_cast?

tautschnig · 2019-01-23T16:54:59Z

src/util/narrow.h

@@ -0,0 +1,34 @@
+// Copyright 2019 Diffblue Limited. All Rights Reserved.


Please sync within Diffblue whether that's a copyright mark to use.

Copyright updated.

tautschnig · 2019-01-23T16:56:14Z

src/util/narrow.h

+  static_assert(
+    std::is_arithmetic<input_type>::value &&
+      std::is_arithmetic<output_type>::value,
+    "numeric_cast is intended only for numeric conversions");


The function is called narrow_cast, but it does make we wonder whether we really need both narrow_cast and numeric_cast(_v)?

Wasn't numeric cast for expr -> mp_integer?

narrow_cast is intended only to suppress compiler warnings. It isn't meant to handle errors. It works exactly like static_cast. But it's easier to grep.

LAJW · 2019-01-23T17:59:41Z

Test-gen PR: https://github.com/diffblue/test-gen/pull/2837

allredj

🚫
This PR failed Diffblue compatibility checks (cbmc commit: b158de9).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/98356611
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

🚫
This PR failed Diffblue compatibility checks (cbmc commit: b648a98).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/98450723
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.

LAJW requested review from NathanJPhillips, cesaro, cristina-david, kroening, martin-cs, peterschrammel, pkesseli, smowton, tautschnig and thk123 as code owners January 23, 2019 16:48

hannes-steffenhagen-diffblue approved these changes Jan 23, 2019

View reviewed changes

hannes-steffenhagen-diffblue reviewed Jan 23, 2019

View reviewed changes

tautschnig approved these changes Jan 23, 2019

View reviewed changes

thk123 approved these changes Jan 23, 2019

View reviewed changes

Lukasz A.J. Wrona added 2 commits January 23, 2019 17:44

Add narrow_cast and narrow

0712104

Add narrow_cast around convesions triggering test-gen

b648a98

allredj reviewed Jan 23, 2019

View reviewed changes

allredj reviewed Jan 24, 2019

View reviewed changes

LAJW merged commit bed39f8 into diffblue:develop Jan 24, 2019

LAJW deleted the lajw/narrow branch January 24, 2019 13:18

owen-mc-diffblue mentioned this pull request Jan 28, 2019

Explicit casts to avoid conversion warnings [blocks: #2310] #2551

Closed

		@@ -0,0 +1,34 @@
		// Copyright 2019 Diffblue Limited. All Rights Reserved.

Conversation

LAJW commented Jan 23, 2019

Uh oh!

tautschnig commented Jan 23, 2019

Uh oh!

hannes-steffenhagen-diffblue left a comment • 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!

hannes-steffenhagen-diffblue Jan 23, 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!

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 commented Jan 23, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hannes-steffenhagen-diffblue left a comment •

edited

Loading

hannes-steffenhagen-diffblue Jan 23, 2019 •

edited

Loading