Skip to content

Make a few changes suggested by @smowton #1471

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 12, 2017
Merged

Make a few changes suggested by @smowton #1471

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions src/util/expr_cast.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ Author: Nathan Phillips <[email protected]>
/// \tparam T The exprt-derived class to check for
/// \param base Reference to a generic \ref exprt
/// \return true if \a base is of type \a T
template<typename T> bool can_cast_expr(const exprt &base);
template<typename T> inline bool can_cast_expr(const exprt &base);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This means missing definitions are flagged at compile time rather than link time.

Copy link
Contributor

Choose a reason for hiding this comment

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

That doesn't look like common practice, I'd put that comment in the code.
Also it looks like can_cast_expr should have been expressed as a meta function. Inline is merely a suggestion for the compiler to inline. It might work differently on debug and release modes and different compilers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not so in this case -- if it's declared inline, then the compiler knows it definitely wants a definition in this unit, rather than generating an extern symbol and waiting for the linker to solve the problem. It's still free to choose whether to actually inline or compile out-of-line and flag it for merging with other units' definitions.


/// Called after casting. Provides a point to assert on the structure of the
/// expr. By default, this is a no-op, but you can provide an overload to
Expand All @@ -45,7 +45,8 @@ struct expr_try_dynamic_cast_return_typet final
{
static_assert(
!std::is_reference<Ret>::value,
"Ret must be non-qualified");
"Ret must not be a reference, i.e. expr_try_dynamic_cast<const thingt> "
"rather than expr_try_dynamic_cast<const thing &>");

typedef
typename std::conditional<
Expand Down Expand Up @@ -78,7 +79,7 @@ auto expr_try_dynamic_cast(TExpr &base)
static_assert(
std::is_base_of<exprt, T>::value,
"The template argument T must be derived from exprt.");
if(!can_cast_expr<T>(base))
if(!can_cast_expr<typename std::remove_const<T>::type>(base))
return nullptr;
const auto ret=static_cast<returnt>(&base);
validate_expr(*ret);
Expand All @@ -93,7 +94,8 @@ struct expr_dynamic_cast_return_typet final
{
static_assert(
!std::is_reference<Ret>::value,
"Ret must be non-qualified");
"Ret must not be a reference, i.e. expr_dynamic_cast<const thingt> rather "
"than expr_dynamic_cast<const thing &>");

typedef
typename std::conditional<
Expand Down