Java object factory: retain tagged types #4161
Conversation
Previously it would follow types as it went, resulting in generated objects with raw struct types. Now it only follows types locally for reference, and passes the tagged types to subroutines.
smowton
requested review from
cesaro,
cristina-david,
NathanJPhillips,
peterschrammel,
pkesseli and
thk123
as code owners
smowton
force-pushed
the
smowton/fix/object-factory-keep-struct-tags
branch
from
d52dcd4 to
7746d32
Compare
|
Test-gen bump (https://github.com/diffblue/test-gen/pull/3036) passing |
| if(target_type.id() == ID_struct) | ||
| const typet &followed_target_type = ns.follow(target_type); | ||
|
|
||
| if(followed_target_type.id() == ID_struct) |
There was a problem hiding this comment.
⛏️ Prefer not directly accessing the id:
if(const auto target_class_type = try_type_dynamic_cast<java_class_type>(followed_target_type))🦅
There was a problem hiding this comment.
Has the advantage that if we choose to tag java_class_type to differentiate from other struct types, will automatically get checked here.
There was a problem hiding this comment.
I'll let the id() / dynamic_cast war work itself out in the other thread before doing this on existing code :)
| if(subtype.id()==ID_struct) | ||
| const typet &subtype = pointer_type.subtype(); | ||
| const typet &followed_subtype = ns.follow(subtype); | ||
| if(followed_subtype.id() == ID_struct) |
| @@ -1049,9 +1054,9 @@ void java_object_factoryt::gen_nondet_init( | |||
| update_in_place, | |||
| location); | |||
| } | |||
| else if(type.id()==ID_struct) | |||
| else if(followed_type.id() == ID_struct) | |||
|
|
||
| THEN("No raw struct expressions should appear") | ||
| { | ||
| REQUIRE(!uses_raw_struct_types(created_code)); |
There was a problem hiding this comment.
⛏️ REQUIRE_FALSE is a little clearer to read than the ! IMO
|
|
||
| THEN("An A, a B and a C object should be allocated") | ||
| { | ||
| REQUIRE(contains_decl_of_type(created_code, A_type)); |
There was a problem hiding this comment.
💡 You might like to use require_goto_statements::require_declaration_of_name either here, or inline, or expand it with your functionality.
|
|
||
| static bool contains_decl_of_type(const exprt &expr, const typet &type) | ||
| { | ||
| return contains(expr, [&type](const exprt &subexpr) { |
There was a problem hiding this comment.
⛏️ Here you do a full tree search of the entire ast for code_decl, but in practise it is only code_blockt you need to flatten? require_goto_statements::get_all_statements does the top level flattening. This has the advantage of not being misleading since a decl can't really appear anywhere in the AST.
| } | ||
|
|
||
| static bool | ||
| contains(const exprt &expr, std::function<bool(const exprt &)> predicate) |
There was a problem hiding this comment.
⛏️ Could these be implemented with a depth_begin()/depth_end()? Then can use std::any?
| THEN("An HasGeneric, Generic and OtherGeneric object should be allocated") | ||
| { | ||
| REQUIRE(contains_decl_of_type(created_code, HasGeneric_type)); | ||
| REQUIRE(contains_decl_with_id(created_code, generic_id)); |
There was a problem hiding this comment.
⛏️ I think this name is misleading: I was expecting it to be the identifier of the declaration, rather than the tag of the type, perhaps a better name would be contains_decl_with_struct_tag?
There was a problem hiding this comment.
Also, why do you handle these via just checking tags rather than through the actual type?
There was a problem hiding this comment.
Laziness -- they're generics so they have decorators (so they're not == to a plain struct_tag_typet). Explained in a comment; I'll leave it to the generics tests to verify the structure of generic types.
Previously if asked to populate a pointer with tagged type (e.g. "Object *") then it would
allocate a "struct Object { ... }", i.e. the struct_typet rather than the struct_tag_typet.
This verifies that it now retains tag types.
smowton
force-pushed
the
smowton/fix/object-factory-keep-struct-tags
branch
from
7746d32 to
328bc04
Compare
|
Applied some of the nice-to-haves, will merge once CI passes. |
There was a problem hiding this comment.
✔️
Passed Diffblue compatibility checks (cbmc commit: 328bc04).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/100597612
The object factory used to follow all types, meaning if you asked it for a tagged type you'd get a raw struct type back. This alters the factory to keep the tag (desirable when tags can carry generic information), and adds a unit test verifying that it doesn't strip the tag in some common unusual cases (array, enumeration and generic initialisation).