Skip to content

Factor class identifier extraction out of remove virtuals #471

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.

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
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
2 changes: 1 addition & 1 deletion src/goto-programs/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ SRC = goto_convert.cpp goto_convert_function_call.cpp \
goto_trace.cpp xml_goto_trace.cpp vcd_goto_trace.cpp \
graphml_witness.cpp remove_virtual_functions.cpp \
class_hierarchy.cpp show_goto_functions.cpp get_goto_model.cpp \
slice_global_inits.cpp goto_inline_class.cpp
slice_global_inits.cpp goto_inline_class.cpp class_identifier.cpp

INCLUDES= -I ..

Expand Down
88 changes: 88 additions & 0 deletions src/goto-programs/class_identifier.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/*******************************************************************\

Module: Extract class identifier

Author: Chris Smowton, [email protected]

\*******************************************************************/

#include "class_identifier.h"

#include <util/std_expr.h>

/*******************************************************************\

Function: build_class_identifier

Inputs: Struct expression

Outputs: Member expression giving the clsid field of the input,
or its parent, grandparent, etc.

Purpose:

\*******************************************************************/

static exprt build_class_identifier(
const exprt &src,
const namespacet &ns)
{
// the class identifier is in the root class
exprt e=src;

while(1)
{
const typet &type=ns.follow(e.type());
assert(type.id()==ID_struct);
Copy link
Collaborator

Choose a reason for hiding this comment

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

That assertion is taken care of by to_struct_type below.


const struct_typet &struct_type=to_struct_type(type);
const struct_typet::componentst &components=struct_type.components();
assert(!components.empty());

member_exprt member_expr(
e,
components.front().get_name(),
components.front().type());

if(components.front().get_name()=="@class_identifier")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit pick: you might want to introduce a temporary for components.front().get_name() as you are accessing it twice.

{
// found it
return member_expr;
}
else
{
e=member_expr;
Copy link
Collaborator

Choose a reason for hiding this comment

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

While I'm nit picking: e.swap(member_expr); should be more efficient.

}
}
}

/*******************************************************************\

Function: get_class_identifier_field

Inputs: Pointer expression of any pointer type, including void*,
and a recommended access type if the pointer is void-typed.

Outputs: Member expression to access a class identifier, as above.

Purpose:

\*******************************************************************/

exprt get_class_identifier_field(
exprt this_expr,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I can see that you modify it below, but passing exprt by value is just uncommon and thus surprising.

const symbol_typet &suggested_type,
const namespacet &ns)
{
// Get a pointer from which we can extract a clsid.
// If it's already a pointer to an object of some sort, just use it;
// if it's void* then use the suggested type.

assert(this_expr.type().id()==ID_pointer &&
"Non-pointer this-arg in remove-virtuals?");
const auto& points_to=this_expr.type().subtype();
Copy link
Collaborator

Choose a reason for hiding this comment

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

"&" goes with points_to

if(points_to==empty_typet())
this_expr=typecast_exprt(this_expr, pointer_typet(suggested_type));
exprt deref=dereference_exprt(this_expr, this_expr.type().subtype());
Copy link
Collaborator

Choose a reason for hiding this comment

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

dereference_exprt deref(this_expr, this_expr.type().subtype()); (I guess the question on why that second argument is required still rests with @kroening).

return build_class_identifier(deref, ns);
}
21 changes: 21 additions & 0 deletions src/goto-programs/class_identifier.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/*******************************************************************\

Module: Extract class identifier

Author: Chris Smowton, [email protected]

\*******************************************************************/

#ifndef CPROVER_GOTO_PROGRAMS_CLASS_IDENTIFIER_H
#define CPROVER_GOTO_PROGRAMS_CLASS_IDENTIFIER_H

#include <util/expr.h>
#include <util/std_types.h>
#include <util/namespace.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit picking: I suppose the debate whether or not to use forward declarations between @peterschrammel and myself never concluded, so I'm still in favour of

class exprt;
class namespacet;
class symbol_exprt;


exprt get_class_identifier_field(
exprt this_expr,
const symbol_typet &suggested_type,
const namespacet &ns);

#endif
65 changes: 6 additions & 59 deletions src/goto-programs/remove_virtual_functions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ Author: Daniel Kroening, [email protected]
#include <util/type_eq.h>

#include "class_hierarchy.h"
#include "class_identifier.h"
#include "remove_virtual_functions.h"

/*******************************************************************\
Expand Down Expand Up @@ -61,8 +62,6 @@ class remove_virtual_functionst
exprt get_method(
const irep_idt &class_id,
const irep_idt &component_name) const;

exprt build_class_identifier(const exprt &);
};

/*******************************************************************\
Expand All @@ -88,48 +87,6 @@ remove_virtual_functionst::remove_virtual_functionst(

/*******************************************************************\

Function: remove_virtual_functionst::build_class_identifier

Inputs:

Outputs:

Purpose:

\*******************************************************************/

exprt remove_virtual_functionst::build_class_identifier(
const exprt &src)
{
// the class identifier is in the root class
exprt e=src;

while(1)
{
const typet &type=ns.follow(e.type());
assert(type.id()==ID_struct);

const struct_typet &struct_type=to_struct_type(type);
const struct_typet::componentst &components=struct_type.components();
assert(!components.empty());

member_exprt member_expr(
e, components.front().get_name(), components.front().type());

if(components.front().get_name()=="@class_identifier")
{
// found it
return member_expr;
}
else
{
e=member_expr;
}
}
}

/*******************************************************************\

Function: remove_virtual_functionst::remove_virtual_function

Inputs:
Expand Down Expand Up @@ -181,22 +138,12 @@ void remove_virtual_functionst::remove_virtual_function(
goto_programt new_code_calls;
goto_programt new_code_gotos;

// Get a pointer from which we can extract a clsid.
// If it's already a pointer to an object of some sort, just use it;
// if it's void* then use the parent of all possible candidates,
// which by the nature of get_functions happens to be the last candidate.

exprt this_expr=code.arguments()[0];
assert(this_expr.type().id()==ID_pointer &&
"Non-pointer this-arg in remove-virtuals?");
const auto &points_to=this_expr.type().subtype();
if(points_to==empty_typet())
{
symbol_typet symbol_type(functions.back().class_id);
this_expr=typecast_exprt(this_expr, pointer_typet(symbol_type));
}
exprt deref=dereference_exprt(this_expr, this_expr.type().subtype());
exprt c_id2=build_class_identifier(deref);
// If necessary, cast to the last candidate function to get the object's clsid.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just had a look at the linter's complaints: it seems this line is too long; could you please also add a commit to address the other 5 concerns on this file, even though they aren't your changes:

src/goto-programs/remove_virtual_functions.cpp:142:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/goto-programs/remove_virtual_functions.cpp:199:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/goto-programs/remove_virtual_functions.cpp:199:  Statement after an if should be on a new line  [readability/braces] [5]
src/goto-programs/remove_virtual_functions.cpp:200:  Statement after an if should be on a new line  [readability/braces] [5]
src/goto-programs/remove_virtual_functions.cpp:305:  Statement after an if should be on a new line  [readability/braces] [5]
src/goto-programs/remove_virtual_functions.cpp:383:  Should have a space between // and comment  [whitespace/comments] [4]

// By the structure of get_functions, this is the parent of all other classes
// under consideration.
symbol_typet suggested_type(functions.back().class_id);
exprt c_id2=get_class_identifier_field(this_expr, suggested_type, ns);

goto_programt::targett last_function;
for(const auto &fun : functions)
Expand Down