Re: Support for function pointers and references

[Ondrej Oprala <ooprala@redhat.com>]

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)
>
> 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());
>        }
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:
>
>      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".
My bad, sorry.
> Thanks!
The changes are in the usual place :)
Thanks for your patience!
 Ondrej