[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Support for function pointers and references
On 29.09.2015 10:58, Dodji Seketeli wrote:
Hello Ondrej,
I have been looking at the second and last patch of the series:
commit 18a700ff0751a0c578a8c5c59abc5c87c8143335
Author: Ondrej Oprala <ooprala@redhat.com>
Date: Wed Sep 23 08:44:00 2015 +0200
Support pointers and references to functions
Again, great patch. Thanks for your hard work.
I have a few comments that you'll find below.
diff --git a/include/abg-ir.h b/include/abg-ir.h
[...]
@@ -1042,6 +1045,7 @@ public:
/// Convenience typedef for a vector of @ref decl_base_sptr.
typedef std::vector<decl_base_sptr > declarations;
+ typedef std::vector<function_type_sptr > function_types;
Please add a comment for this typedef (starting with "///") so that
the apidoc is updated, just like what we do for the other typedefs. I
agree this commenting business feels cumbersome but then later we're
happy when we enjoy the completeness of the apidoc :-)
[...]
@@ -5986,7 +5986,6 @@ compute_diff_for_types(const decl_base_sptr first,
||(d = try_to_diff<array_type_def>(f, s, ctxt))
||(d = try_to_diff<qualified_type_def>(f, s, ctxt))
||(d = try_to_diff<typedef_decl>(f, s, ctxt))
- ||(d = try_to_diff<function_type>(f, s, ctxt))
||(d = try_to_diff_distinct_kinds(f, s, ctxt)));
Why avoid handling function types here, to just handle them ...
@@ -7138,9 +7137,21 @@ compute_diff(pointer_type_def_sptr first,
if (first && second)
assert(first->get_environment() == second->get_environment());
- diff_sptr d = compute_diff_for_types(first->get_pointed_to_type(),
- second->get_pointed_to_type(),
- ctxt);
+ diff_sptr d;
+
+ function_type_sptr f = dynamic_pointer_cast<function_type>(first->get_pointed_to_type()),
+ g = dynamic_pointer_cast<function_type>(second->get_pointed_to_type());
+ if (f && g)
+ d = compute_diff(f, g, ctxt);
+ else if (f || g)
+ d = try_to_diff_distinct_kinds(first->get_pointed_to_type(),
+ second->get_pointed_to_type(),
+ ctxt);
+ else
+ d = compute_diff_for_types(first->get_pointed_to_type(),
+ second->get_pointed_to_type(),
+ ctxt);
+
... here ?
I would think that compute_diff_for_type() would handle the two
pointer types fine; then the overload of compute_diff() for pointer
types would call compute_diff_for_type() on the pointed-to types,
again; then compute_diff_for_type() *should* handle all the cases of
pointed-to types; if not, it should extended to do so. What am I
missing?
You're right, fixed now.
diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc
[...]
+ /* For now, we skip the hidden vtable pointer */
+ if (n.substr(0, 5) == "_vptr"
+ && !std::isalnum(n.at(5))
+ && n.at(5) != '_')
+ continue;
Please add a comment explaining the (name) pattern you are looking for
to recognize the vtable pointer member here. In these matters, I
believe redundancy of information is helpful to catch issues later.
- utype_decl =
- dynamic_pointer_cast<decl_base>(build_ir_node_for_void_type(ctxt));
Hehe, see, I was using the dynamic_pointer_cast, rather than using the
'is_decl()' function I told you about earlier. Bad bad me. :-)
It's cool how your changes removed those, by the way.
Initially the code base had a lot of these; but then after I have
started to debug the code base a little bit more, it appeared that using
the is_*() function was much more practical. So I introduced them in
new code, updated some old code, but obviously, some old code still
needs to be updated. I think we'll need to provide some patches later
to update the code towards that end.
[...]
+static function_type_sptr
+build_function_type(read_context& ctxt,
+ Dwarf_Die* die,
+ bool die_is_from_alt_di,
+ class_decl_sptr is_method,
+ size_t where_offset)
+{
[...]
+ return_type_decl = dynamic_pointer_cast<decl_base>(
+ build_ir_node_from_die(ctxt, &ret_type_die,
+ ret_type_die_is_in_alternate_debug_info,
+ /*called_from_public_decl=*/true,
+ where_offset));
Please use is_decl() here.
[...]
+ if (!result && dwarf_child(die, &child) == 0)
+ do
+ {
It seems to me the !result clause is useless here; is it not?
I understand that you took this code from the former
build_function_decl() code; in that former context, the !result clause
was not useless because the 'result' could have been initialized to a
non-nil declaration. But here, I don't see that.
Oops, you're right
[...]
+ parm_type_decl = dynamic_pointer_cast<decl_base>(
+ build_ir_node_from_die(ctxt, &parm_type_die,
+ parm_type_die_is_in_alt_di,
+ /*called_from_public_decl=*/true,
+ where_offset));
Please use is_decl() rather than the dynamic_pointer_cast.
[...]
+ }
+ while (!result && dwarf_siblingof(&child, &child) == 0);
Here, I think the !result clause is useless as well.
By the way, build_function_type() is cool, I like it :-)
and using it here ...
@@ -7315,75 +7447,10 @@ build_function_decl(read_context& ctxt,
[...]
+ function_type_sptr fn_type(build_function_type(ctxt,
+ die,
+ is_in_alt_di,
+ is_method,
+ where_offset));
looks very natural much more than what I was doing. That is super cool.
thank you for that!
[...]
diff --git a/src/abg-ir.cc b/src/abg-ir.cc
[...]
+/// Getter of the the function types of the current @ref translation
+/// unit.
There is a "the the" repetition there. Also, it should be @ref translation_unit.
[...]
@@ -6541,16 +6549,16 @@ pointer_type_def::pointer_type_def(const type_base_sptr&
[...]
+ if (pto)
+ set_visibility(pto->get_visibility());
+ pointed_to_type_ = weak_ptr<type_base>(type_or_void(pointed_to, 0));
Please use the type_base_wptr typedef here, rather than the longer
weak_ptr<type_base> notation.
- name = "void";
+ name = get_type_name(get_pointed_to_type(), /*qualified_name=*/true);
It looks like there is a white space issue here. Normally, before the
"name" token, there should be one tab, not 8 white spaces.
[...]
+ else
+ name = get_type_name(dynamic_pointer_cast<function_type>(pointed_to),
+ /*qualified_name=*/true) + "&";
Please use is_function_type() here, rather than
dynamic_pointer_cast().
[...]
+
+ pointed_to_type_ = weak_ptr<type_base>(type_or_void(pointed_to, 0));
Please use type_base_wptr.
[...]
@@ -6808,7 +6825,10 @@ reference_type_def::get_qualified_name() const
get_type_declaration(type_or_void(get_pointed_to_type(),
get_environment()));
string name;
- td->get_qualified_name(name);
+ if (!td)
+ name = get_type_name(get_pointed_to_type(), /*qualified_name=*/true);
+ else
+ td->get_qualified_name(name);
I think we could do away with 'td' completely. I mean Let's not even
try to get the type declaration here. Let's just use get_type_name()
as it knows how to handle types that don't have declarations. It'll
make the logic simpler and thus easier to maintain later.
[...]
diff --git a/src/abg-writer.cc b/src/abg-writer.cc
[...]
@@ -1232,6 +1266,9 @@ write_pointer_type_def(const pointer_type_def_sptr decl
[...]
+ if (dynamic_pointer_cast<function_type>(decl->get_pointed_to_type()))
+ ctxt.record_type_as_emitted(decl->get_pointed_to_type());
+
I think that as the pointed to type is *not* written here, it should
not be marked as being written.
My understanding is that the reason why you are doing this hack is that
only function types that are referenced via function pointers should be
emitted. Other functions types that are not referenced by function
pointers should not be emitted. Is that the case?
If so, what you could do instead, is to keep a (well documented) map of
"function types to emit", on the side. Note that the key of the map
would be the pointer *value* of the canonical type of the function type.
So when you are here:
@@ -971,6 +991,20 @@ write_translation_unit(const translation_unit& tu,
[...]
+ typedef scope_decl::function_types function_types;
+ typedef function_types::const_iterator const_fn_iterator;
+ const function_types& t = tu.get_function_types();
+
+ for (const_fn_iterator i = t.begin(); i != t.end(); ++i)
+ {
+ if (!ctxt.type_is_emitted(dynamic_pointer_cast<type_base>(*i)))
+ // This type has already been written out to the current
+ // translation unit, so do not emit it again.
+ continue;
+ o << "\n";
+ write_function_type(*i, ctxt, indent + c.get_xml_element_indent());
+ }
+
, you can then check that the function type you are about to emit is
part of the map of function types to emit. If it is, then emit it
(that would mark the function type as having been emitted) and remove
it from the map of function pointers to emit. You can thus assert
that a function type that is in the map of the function types to emit
has *not* been emitted yet.
Then you can remove this:
+ void
+ record_type_id_as_not_emitted(const string& id)
+ {m_emitted_type_id_map.erase(id);}
, this,
+ void
+ record_type_as_not_emitted(const type_base_sptr& t)
and this (in the new write_function_type()):
+ ctxt.record_type_as_not_emitted(decl);
Yeah, I tried to hack around it using existing code...should be much
better now.
Cheers,
I've rebased, fixed and amended both patches and they're in
ondrej/fnptrs again. Thank you for your review!
Cheers,
Ondrej