[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Support for function pointers and references

On 30.09.2015 09:10, Dodji Seketeli wrote:
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.
Cool, thanks!
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)


     record_fntype_as_referenced(const function_type_sptr& f)

I'd also rename:
     +  bool
     +  fntype_to_emit(const function_type_sptr& f)


     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());
     +    }


     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.
	o << "\n";
	write_function_type(*i, ctxt, indent + c.get_xml_element_indent());
I've followed your renames, but I've made the condition

if (!ctxt.fntype_is_referenced(*i) || ctxt.type_is_emitted(*i))

...and amended the comment accordingly...as otherwise it would let unreferenced fntypes slip through.
I also created a ctxt.clear_referenced_fntypes_map, since it needs to be cleared
along with the emitted_types_map, otherwise we'd get duplicates across TUs.

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:


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".
My bad, sorry.

The changes are in the usual place :)
Thanks for your patience!