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

Re: Support for function pointers and references

Hi Dodji,
thank you for a lightning fast review!
I'll amend it and rebase against master.
I'll ping you on IRC when it's done.

Thanks again!

On 28.09.2015 16:44, Dodji Seketeli wrote:
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,
- 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

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


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

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.