Skip to content

Enable perfect forwarding with irept/exprt/typet constructors #3502

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 7 commits into from
Mar 14, 2019
Merged

Enable perfect forwarding with irept/exprt/typet constructors #3502

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c61a3ba
Move constructor/assignment for dstringt
tautschnig Mar 4, 2019
027760b
Pass irept by value in modifying irept::add/set operations
tautschnig Dec 2, 2018
60e9284
Add pass-by-value constructor to exprt
tautschnig Dec 2, 2018
c7d38df
Pass-by-value constructors for typet and type_with_subtypet
tautschnig Dec 2, 2018
c267421
Pass-by-value in *exprt constructors
tautschnig Feb 11, 2019
3924933
Pass-by-value in *typet constructors
tautschnig Feb 11, 2019
83ed9a5
Pass-by-value in code*t constructors
tautschnig Feb 11, 2019
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
20 changes: 20 additions & 0 deletions src/util/dstring.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,18 @@ class dstringt final
{
}

dstringt(const dstringt &) = default;

/// Move constructor. There is no need and no point in actually destroying the
/// source object \p other, this is effectively just a copy constructor.
#ifdef __GNUC__
constexpr
#endif
dstringt(dstringt &&other)
: no(other.no)
{
}

// access

bool empty() const
Expand Down Expand Up @@ -136,6 +148,14 @@ class dstringt final
dstringt &operator=(const dstringt &b)
{ no=b.no; return *this; }

/// Move assignment. There is no need and no point in actually destroying the
/// source object \p other, this is effectively just an assignment.
dstringt &operator=(dstringt &&other)
{
no = other.no;
return *this;
}

// output

std::ostream &operator<<(std::ostream &out) const;
Expand Down
21 changes: 18 additions & 3 deletions src/util/expr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,18 @@ class exprt:public irept
// constructors
exprt() { }
explicit exprt(const irep_idt &_id):irept(_id) { }
exprt(const irep_idt &_id, const typet &_type):irept(_id)

exprt(irep_idt _id, typet _type)
: irept(std::move(_id), {{ID_type, std::move(_type)}}, {})
{
}

exprt(irep_idt _id, typet _type, operandst &&_operands)
: irept(
std::move(_id),
{{ID_type, std::move(_type)}},
Copy link
Member

Choose a reason for hiding this comment

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

moving a dstring may well be slower than copying it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will remove.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Or actually: shouldn't we just define a move constructor for dstringt that actually is cheap? For the case that irep_idt == std::string we'd still want the std::move.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have added a commit that adds efficient move construction/assignment to dstringt.

Copy link
Member

Choose a reason for hiding this comment

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

I am still not entirely convinced that we need those std::move(id) -- we wouldn't do it for an integer, say.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But we don't know whether irep_idt is a std::string (where it would seem worthwhile) or a dstringt (where it's certainly unnecessary). But also that isn't something I want this PR to get blocked on, I'm happy to follow whatever I'm advised to do.

std::move((irept::subt &&) _operands))
{
add(ID_type, _type);
}

/// Return the type of the expression
Expand Down Expand Up @@ -324,7 +333,13 @@ class expr_protectedt : public exprt
{
protected:
// constructors
expr_protectedt(const irep_idt &_id, const typet &_type) : exprt(_id, _type)
expr_protectedt(irep_idt _id, typet _type)
: exprt(std::move(_id), std::move(_type))
{
}

expr_protectedt(irep_idt _id, typet _type, operandst _operands)
: exprt(std::move(_id), std::move(_type), std::move(_operands))
{
}

Expand Down
14 changes: 8 additions & 6 deletions src/util/irep.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ irept &irept::add(const irep_namet &name)
#endif
}

irept &irept::add(const irep_namet &name, const irept &irep)
irept &irept::add(const irep_namet &name, irept irep)
{
named_subt &s = get_named_sub();

Expand All @@ -202,18 +202,20 @@ irept &irept::add(const irep_namet &name, const irept &irep)
named_subt::iterator before = s.before_begin();
while(std::next(before) != it)
++before;
it = s.emplace_after(before, name, irep);
it = s.emplace_after(before, name, std::move(irep));
}
else
it->second=irep;
it->second = std::move(irep);

return it->second;
#else
std::pair<named_subt::iterator, bool> entry=
s.insert(std::make_pair(name, irep));
std::pair<named_subt::iterator, bool> entry = s.emplace(
std::piecewise_construct,
std::forward_as_tuple(name),
std::forward_as_tuple(irep));

if(!entry.second)
entry.first->second=irep;
entry.first->second = std::move(irep);

return entry.first->second;
#endif
Expand Down
12 changes: 8 additions & 4 deletions src/util/irep.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,7 @@ class irept

const irept &find(const irep_namet &name) const;
irept &add(const irep_namet &name);
irept &add(const irep_namet &name, const irept &irep);
irept &add(const irep_namet &name, irept irep);

Copy link
Member

Choose a reason for hiding this comment

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

One variant with just irept suffices.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

const std::string &get_string(const irep_namet &name) const
{
Expand All @@ -435,9 +435,13 @@ class irept
long long get_long_long(const irep_namet &name) const;

void set(const irep_namet &name, const irep_idt &value)
{ add(name).id(value); }
void set(const irep_namet &name, const irept &irep)
{ add(name, irep); }
{
add(name, irept(value));
}
void set(const irep_namet &name, irept irep)
{
add(name, std::move(irep));
}
void set(const irep_namet &name, const long long value);
Copy link
Member

Choose a reason for hiding this comment

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

Since this will copy eventually, simply have one variant, with just irept, and then move that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.


void remove(const irep_namet &name);
Expand Down
8 changes: 3 additions & 5 deletions src/util/ssa_expr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ class ssa_exprt:public symbol_exprt
public:
/// Constructor
/// \param expr: Expression to be converted to SSA symbol
explicit ssa_exprt(const exprt &expr) : symbol_exprt(irep_idt(), expr.type())
explicit ssa_exprt(const exprt &expr) : symbol_exprt(expr.type())
{
Copy link
Member

Choose a reason for hiding this comment

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

I kind of liked that symbol_exprt now enforces that you set the identifier.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think symbol_exprt actually does? This change here just removes an unnecessary call to set_identifier.

Copy link
Member

Choose a reason for hiding this comment

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

No, it doesn't, but I was hoping it would eventually.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can certainly put the irep_idt() back if and when we do.

set(ID_C_SSA_symbol, true);
add(ID_expression, expr);
Expand All @@ -42,15 +42,13 @@ class ssa_exprt:public symbol_exprt
if(original_expr.id() == ID_symbol)
return to_symbol_expr(original_expr).get_identifier();

object_descriptor_exprt ode;
ode.object() = original_expr;
object_descriptor_exprt ode(original_expr);
return to_symbol_expr(ode.root_object()).get_identifier();
}

const ssa_exprt get_l1_object() const
{
object_descriptor_exprt ode;
ode.object()=get_original_expr();
object_descriptor_exprt ode(get_original_expr());

ssa_exprt root(ode.root_object());
root.set(ID_L0, get(ID_L0));
Expand Down
Loading