Add try_dynamic_cast from rvalue to `optionalt` #4028

thomasspriggs · 2019-02-02T16:06:40Z

This pr adds templates for expr_try_dynamic_cast and
type_try_dynamic_cast where the parameter is an rvalue and the return
type is an optionalt. This is implemented by moving the parameter into
the optionalt.

Included are unit tests of the new templates, which show that they
return the types and values expected. As well as tests and a static
assert for the existing overloads which show that they still return a
pointer.

These new templates are useful in the case where we are using the
result of a function, which returns by value but only in the case where
the value can be cast to a given type. For example with the new
overloads the following code can be written -

exprt enig();
void handle_struct_case(const struct_exprt &struct_expr);

void myFunction()
{
  if(const auto my_struct = expr_try_dynamic_cast<struct_exprt>(enig()))
    handle_struct_case(*my_struct);
}

However without the new templates and because the old ones do not bind
to rvalues, an additional temporary variable otherwise would have to be
declared -

void myFunction2()
{
  const exprt enigma = enig();
  if(const auto my_struct = expr_try_dynamic_cast<struct_exprt>(enigma))
    handle_struct_case(*my_struct);
}

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.
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).
My commit message includes data points confirming performance improvements (if claimed).
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-02-02T16:26:23Z

src/util/expr_cast.h

@@ -107,6 +107,32 @@ auto expr_try_dynamic_cast(TExpr &base)
  return ret;
 }

+/// \brief Try to cast a generic exprt to a specific derived class.
+/// \tparam T: The type to cast the `base` param to.
+/// \tparam TType: The original type to cast from, Must be a exprt rvalue.


Nit pick: s/Must/must/

tautschnig · 2019-02-02T16:33:45Z

src/util/expr_cast.h

@@ -107,6 +107,32 @@ auto expr_try_dynamic_cast(TExpr &base)
  return ret;
 }

+/// \brief Try to cast a generic exprt to a specific derived class.
+/// \tparam T: The type to cast the `base` param to.


Nit pick: I'd use \p base (it will be rendered the same way, but conveys the fact that this is a parameter)

tautschnig · 2019-02-02T16:35:42Z

src/util/expr_cast.h

+    return {};
+  optionalt<T> ret{static_cast<T &&>(base)};
+  validate_expr(*ret);
+  return ret;


It probably doesn't make much of a difference, other than saving one line, but I'd have used:

validate_expr(base); return std::move(base);

tautschnig · 2019-02-02T16:36:08Z

src/util/expr_cast.h

@@ -134,6 +160,31 @@ auto type_try_dynamic_cast(TType &base) ->
  return ret;
 }

+/// \brief Try to cast a generic typet to a specific derived class.
+/// \tparam T: The type to cast the `base` param to.
+/// \tparam TType: The original type to cast from, Must be a typet rvalue.


All the same comments as above apply...

smowton

Looks good, one nitpick

smowton · 2019-02-04T12:17:37Z

unit/util/expr_cast/expr_cast.cpp

+  {
+    typet type = string_type;
+
+    THEN("Trying casting a value to a string_typet should yield a non null "


a value -> a string type throughout?

No. I think the update should be a value -> a typet lvalue, in order to make things clear.

This commit adds templates for `expr_try_dynamic_cast` and `type_try_dynamic_cast` where the parameter is an rvalue and the return type is an `optionalt`. This is implemented by moving the parameter into the `optionalt`. Included are unit tests of the new templates, which show that they return the types and values expected. As well as tests and a static assert for the existing overloads which show that they still return a pointer. These new templates are useful in the case where we are using the result of a function, which returns by value but only in the case where the value can be cast to a given type. For example with the new overloads the following code can be written - ``` exprt enig(); void handle_struct_case(const struct_exprt &struct_expr); void myFunction() { if(const auto my_struct = expr_try_dynamic_cast<struct_exprt>(enig())) handle_struct_case(*my_struct); } ``` However without the new templates and because the old ones do not bind to rvalues, an additional temporary variable otherwise would have to be declared - ``` void myFunction2() { const exprt enigma = enig(); if(const auto my_struct = expr_try_dynamic_cast<struct_exprt>(enigma)) handle_struct_case(*my_struct); } ```

allredj

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

thomasspriggs requested review from kroening, peterschrammel, pkesseli, smowton and tautschnig as code owners February 2, 2019 16:06

tautschnig approved these changes Feb 2, 2019

View reviewed changes

tautschnig assigned thomasspriggs Feb 2, 2019

thomasspriggs requested a review from NathanJPhillips February 4, 2019 12:12

smowton approved these changes Feb 4, 2019

View reviewed changes

thomasspriggs force-pushed the tas/optional_cast branch from 6fd8653 to 52e95a6 Compare February 5, 2019 12:29

allredj reviewed Feb 5, 2019

View reviewed changes

thomasspriggs merged commit 88ab7d5 into diffblue:develop Feb 5, 2019

thomasspriggs deleted the tas/optional_cast branch February 5, 2019 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add try_dynamic_cast from rvalue to `optionalt` #4028

Add try_dynamic_cast from rvalue to `optionalt` #4028

Uh oh!

thomasspriggs commented Feb 2, 2019 •

edited

Loading

Uh oh!

tautschnig Feb 2, 2019

Uh oh!

tautschnig Feb 2, 2019

Uh oh!

tautschnig Feb 2, 2019

Uh oh!

tautschnig Feb 2, 2019

Uh oh!

smowton left a comment

Uh oh!

smowton Feb 4, 2019

Uh oh!

thomasspriggs Feb 5, 2019

Uh oh!

allredj left a comment

Uh oh!

Uh oh!

Add try_dynamic_cast from rvalue to optionalt #4028

Add try_dynamic_cast from rvalue to optionalt #4028

Uh oh!

Conversation

thomasspriggs commented Feb 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tautschnig Feb 2, 2019

Choose a reason for hiding this comment

Uh oh!

tautschnig Feb 2, 2019

Choose a reason for hiding this comment

Uh oh!

tautschnig Feb 2, 2019

Choose a reason for hiding this comment

Uh oh!

tautschnig Feb 2, 2019

Choose a reason for hiding this comment

Uh oh!

smowton left a comment

Choose a reason for hiding this comment

Uh oh!

smowton Feb 4, 2019

Choose a reason for hiding this comment

Uh oh!

thomasspriggs Feb 5, 2019

Choose a reason for hiding this comment

Uh oh!

allredj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Add try_dynamic_cast from rvalue to `optionalt` #4028

Add try_dynamic_cast from rvalue to `optionalt` #4028

thomasspriggs commented Feb 2, 2019 •

edited

Loading