Re: Support for function pointers and references

[Dodji Seketeli <dodji@seketeli.org>]

Hello Ondrej,

So I have been looking at this patch:

    commit 5e9187ac43824a79c3e3d55ff71503aeb6718c41
    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>


I just have some nitpicking comments about it :-)

[...]

@@ -6570,11 +6570,11 @@ build_class_type_and_add_to_ir(read_context&	ctxt,
 				     type_die_is_in_alternate_debug_info))
 		continue;
 
-	      decl_base_sptr base_type =
+	      decl_base_sptr base_type = dynamic_pointer_cast<decl_base>(

Just as a matter of style, I'd prefer that you use the is_decl()
function (declared in include/abg-fwd.h), which basically does the
dynamic_pointer_cast here, but which is arguably more legible, easier to
call from the debugger (should the need arise) and more importantly, is
what is used throughout the code base for this purpose.

The same comment applies to the other similar calls to the
dynamic_pointer_cast operator to cast type_or_decl_base_sptr to
decl_base_sptr.

 		build_ir_node_from_die(ctxt, &type_die,
 				       type_die_is_in_alternate_debug_info,
 				       called_from_public_decl,
-				       where_offset);
+				       where_offset));

[...]

@@ -6892,8 +6895,8 @@ build_pointer_type_def(read_context&	ctxt,
       assert(result);
       return result;
     }
-
   type_base_sptr utype = is_type(utype_decl);
+
   assert(utype);

I think this is a useless white space change.

This patch looks good to me with those changes.  I can commit it to
master once you've amended it, or you can do it yourself, as you wish
:-)

Thank you again for working on this.

Cheers,

-- 
		Dodji