[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Support for function pointers and references
Hello Ondrej,
I have looked at the new revisions of your patches, please find my
comments below.
The patch:
commit 116a65c65335d731cdae1873cbce9bd338906a90
Author: Ondrej Oprala <ooprala@redhat.com>
Date: Wed Sep 9 10:12:03 2015 +0200
Generalize some dwarf-reader functions to generate and return
instances of type_or_decl_base_stpr to be able to propagate
types occurring without an accompanying declaration.
* src/abg-dwarf-reader.cc (build_ir_node_from_die): Return
a type_or_decl_base_sptr instead.
(get_scope_for_die): Likewise.
(build_class_type_and_add_to_ir): Typecast the assignment from
build_ir_node_from_die properly.
(build_{qualified,reference,array,typedef}_type): Likewise.
(build_pointer_type_def): Likewise.
(build_{var,function}_decl): Likewise.
Signed-off-by: Ondrej Oprala <ooprala@redhat.com>
Looks good to me. It's OK to go in.
For the patch:
commit 18a700ff0751a0c578a8c5c59abc5c87c8143335
Author: Ondrej Oprala <ooprala@redhat.com>
Date: Wed Sep 23 08:44:00 2015 +0200
Support pointers and references to functions
I still have a few comments. Please find them below.
diff --git a/src/abg-writer.cc b/src/abg-writer.cc
[...]
+/// A convenience typedef for a map that associates a shared pointer to a type
+/// to a bool.
There is a little repetition in "to a type to a bool" here.
[...]
@@ -132,6 +138,9 @@ class write_context
ostream& m_ostream;
type_ptr_map m_type_id_map;
unordered_map<string, bool> m_emitted_type_id_map;
[...]
+ /// A vector of canonical types of function types that
+ /// pointed or referenced to in the current TU
+ fn_shared_ptr_map m_fntype_to_emit_map;
I would call this member m_referenced_fntypes_map. Also, I would
rather use the comment:
/// A vector of function types that are referenced by emitted pointers
/// or reference, i.e, that are pointed-to types of pointers or references
/// that are emitted.
Then I would rename:
+ void
+ record_fntype_to_emit(const function_type_sptr& f)
into:
record_fntype_as_referenced(const function_type_sptr& f)
I'd also rename:
+ bool
+ fntype_to_emit(const function_type_sptr& f)
into:
bool
fntype_is_referenced(const function_type_sptr& f)
So, in write_translation_unit, I'd replace the new:
+ for (const_fn_iterator i = t.begin(); i != t.end(); ++i)
+ {
+ if (!ctxt.fntype_to_emit(*i))
+ // This function type wasn't marked as one to be emitted
+ // so skip it.
+ continue;
+ o << "\n";
+ write_function_type(*i, ctxt, indent + c.get_xml_element_indent());
+ }
+
with:
for (const_fn_iterator i = t.begin(); i != t.end(); ++i)
{
if ((ctxt.fntype_is_referenced(*i) && ctxt.type_is_emitted(*i))
// This function type, referenced by an emitted pointer or
// reference type, has already been emitted, so skip it.
continue;
o << "\n";
write_function_type(*i, ctxt, indent + c.get_xml_element_indent());
}
The idea is to make the code reflect exactly what we are thinking
about doing, which is, we want function types that are referenced by
pointer (or reference) types to be emitted; other function types
should not be emitted.
[...]
+static bool
+write_function_type(const shared_ptr<function_type> decl, write_context& ctxt,
+ unsigned indent)
+{
[...]
+ ctxt.record_fntype_as_emitted(decl);
I'd replace this call by a call to the usual
ctxt.record_type_as_emitted() that is used throughout the code:
ctxt.record_type_as_emitted(decl)
I'd then remove the new member write_context::record_fntype_as_emitted()
that you've added.
Also, the new test material (you now, the new test files) need to be
added to tests/data/Makefile.am, otherwise they are not added to the
tarball by "make dist"; and "make distcheck" would fail anyhow. As a
general rule, all patches should pass "make distcheck".
Thanks!
--
Dodji