Skip to content

Add code_typet::get_this #1689

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
Jan 5, 2018
Merged

Add code_typet::get_this #1689

Show file tree
Hide file tree
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
5 changes: 3 additions & 2 deletions src/goto-programs/remove_virtual_functions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,10 +181,11 @@ void remove_virtual_functionst::remove_virtual_function(
// Cast the `this` pointer to the correct type for the new callee:
const auto &callee_type=
to_code_type(ns.lookup(fun.symbol_expr.get_identifier()).type);
const code_typet::parametert *this_param = callee_type.get_this();
INVARIANT(
callee_type.has_this(),
this_param != nullptr,
"Virtual function callees must have a `this` argument");
typet need_type=callee_type.parameters()[0].type();
typet need_type=this_param->type();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be a const reference (given we discussed the cost of copy in this PR...).

if(!type_eq(newcall.arguments()[0].type(), need_type, ns))
newcall.arguments()[0].make_typecast(need_type);
}
Expand Down
10 changes: 9 additions & 1 deletion src/util/std_types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -817,9 +817,17 @@ class code_typet:public typet
}

bool has_this() const
{
return get_this() != nullptr;
}

const parametert *get_this() const
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't return a pointer. I'd suggest to a nil_exprt or an optional<parametert> instead.

Copy link
Contributor Author

@smowton smowton Jan 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

We'd want a reference, so it would have to be an optionalt<std::reference_wrapper<parametert>>, which is considerably messier to work with but has essentially the same meaning as a pointer. There was considerable back-and-forth over this same question in the recent symbol table interface improvements, and the outcome was to allow a non-owning pointer like this with nullptr for not found, e.g. https://github.com/diffblue/cbmc/blob/develop/src/util/symbol_table.h#L96. Therefore I'm inclined to maintain the same pattern here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does it have to be a reference? Can't you just return a parametert by value here (a key difference to the get_writable case)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure? Just seemed wasteful to copy it when that's probably unnecessary.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure this is on a hot path? Copying an irept is pretty cheap, after all.

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 particular battle was last fought here: #1462. IIRC the actual conversation was had with Daniel in person, and that PR adds the result to the coding standard doc.

Every time someone has to return some "not found / undefined" value there is a tussle between irept(), optionalt<...>::none and nullptr, with as far as I can see nothing to recommend any option over any other except consistency, hence my copying the style eventually settled on in symbol_tablet.

If you continue to object I'll just do whatever you say to get this over with, but I'd like to decide once and for all rather than repeat this conversation every time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Admittedly, I completely missed #1462, and in particular its addition to the coding standard (and of course the offline discussion). I have thus removed my block.

Here are my thoughts on the matter:

  1. Avoid pointers wherever possible, as their unchecked use may result in undefined behaviour. With null pointers there will likely be a reliable crash with little exploitability, so the situation isn't terrible, but it's not as great as with references.
  2. If you are in the situation that a value might not be found, then distinguish the following two cases:
    a) Is a missing value an exceptional case? Use some form of INVARIANT (and a reference or some other supposedly-initialised type) or optionalt. Notably, the user of such a method should not be expected to sanity-check the return value, but instead should be able to use it directly.
    b) A missing value is an perfectly normal case. Use a pointer or a nil_exprt (the latter being the more common solution across the code base, I believe -- but really this should be unified), or maybe optionalt. Make clear in comments that the user needs to sanity-check the return value, though hopefully the user is well aware of that anyway.

Now I'm not sure whether this actually adds clarity? Anyway it's just my thoughts, no claim of this being consensus.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re: 2, this is definitely an expected failure case (e.g. the function is a free, non-member function)

{
const parameterst &p=parameters();
return !p.empty() && p.front().get_this();
if(!p.empty() && p.front().get_this())
return &p.front();
else
return nullptr;
}

bool is_KnR() const
Expand Down