[PATCH] Report location info with human-readable(non-xml) output

[PATCH] Report location info with human-readable(non-xml) output

[Ondrej Oprala]

Hi, based on IRC conversations, I wrote a patch for this.
It's pushed to ondrej/locinfo, in case anyone'd like to review it ;) .

Thanks,
    Ondrej

Re: [PATCH] Report location info with human-readable(non-xml) output

[Dodji Seketeli]

Hey Ondrej!

Ondrej Oprala <ooprala@redhat.com> a écrit:

> Hi, based on IRC conversations, I wrote a patch for this.
> It's pushed to ondrej/locinfo, in case anyone'd like to review it ;) .

I am sorry that I took so long to look at this patch of yours.  It looks
good to me.  I just have a few nits.  If you agree with them, then I
pre-approve the patch to be committed in master once you've addressed my
comments.

Thank you a lot for working on this!

[...]

> diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc

> +bool
> +diff_context::show_locs() const
> +{return priv_->show_locs_;}
> +
> +void
> +diff_context::show_locs(bool f)
> +{priv_->show_locs_= f;}
> +

The two accessors above obvious, but I'd like them to be commented
nonetheless.  I realize some accessors around here are not commented,
but that is a mistake that ought to be fixed, I believe.

[...]

> @@ -6600,6 +6620,46 @@ report_size_and_alignment_changes(type_or_decl_base_sptr	first,
>    return false;
>  }
>  
> +/// @param type the type to emit loc info about

I think this function should work on either a type or a decl.  More
precisely, the ABI artifacts that have location information are
declarations.  And we can get the declaration of a type.  I see that
the function  already takes a type_or_decl_base_sptr, and that is
great; it's what it should take.  We'd need to change the parameter
name and the comment.  So, the parameter name could be something like
"tod" (type or declaration) and the comment would be:

 @param tod the type or declaration to emit loc info about.

> +///
> +/// @param ctxt the content of the current diff.
> +///
> +/// @param out the output stream to report the change to.
> +///
> +/// @return true iff something was reported.
> +static bool
> +report_loc_info(type_or_decl_base_sptr type,

Here, I'd rather pass a const type_or_decl_base_sptr&.


> +		const diff_context_sptr& ctxt,

I think this parameter should be a const diff_context& because nothing
in the code of the function seems to require that the context be a
pointer.  It could then be a reference.  And by passing a reference,
we avoid later issues do to callers inadvertently passing a nil
context pointer.

> +		ostream &out)
> +{
> +  if (!ctxt->show_locs())
> +    return false;
> +
> +  decl_base_sptr decl = dynamic_pointer_cast<decl_base>(type);

Please use is_decl().

> +  if (!decl)
> +    decl = get_type_declaration(dynamic_pointer_cast<type_base>(type));

My understanding is that if the previous is_decl() doesn't work,
get_type_declaration() won't work either, because it just does the
same thing as is_decl(), for now.

So the if (!decl) above seems unnecessary.

> +
> +  if (!decl)
> +    return false;
> +
> +  location loc;
> +  translation_unit* tu = get_translation_unit(decl);
> +
> +  if (decl && tu && (loc = decl->get_location()))

checking that 'decl' is non nil here is unnecessary because we are
sure it is not.

> +  {
> +    string path;
> +    unsigned line, column;
> +
> +    tu->get_loc_mgr().expand_location(loc, path, line, column);
> +    path = basename(const_cast<char*>(path.c_str()));
> +
> +    out  << " at " << path << ":" << line << ":" << column;
> +
> +    return true;
> +  }
> +  return false;
> +}
> +

[...]

> @@ -6265,7 +6275,7 @@ maybe_report_diff_for_symbol(const elf_symbol_sptr&	symbol1,
>  ///
>  /// @param out the output stream to send the representation to
>  static void
> -represent(diff_context& ctxt,
> +represent(const diff_context_sptr& ctxt,

As report_loc_info now takes a reference to the diff_context, this
change now becomes unnecessary.  Or, rather, changing the
diff_context& to a const diff_context& would be better.  What do you
think?

I 
>  	  class_decl::method_decl_sptr mem_fn,
>  	  ostream& out)
>  {
> @@ -6277,13 +6287,14 @@ represent(diff_context& ctxt,
>    assert(meth);
>  
>    out << "'" << mem_fn->get_pretty_representation() << "'";
> +  report_loc_info(meth, ctxt, out);
>    if (get_member_function_is_virtual(mem_fn))
>      out << ", virtual at voffset "
>  	<< get_member_function_vtable_offset(mem_fn)
>  	<< "/"
>  	<< meth->get_type()->get_class_type()->get_virtual_mem_fns().size();
>  
> -  if (ctxt.show_linkage_names()
> +  if (ctxt->show_linkage_names()

This change becomes unnecessary as well. There are other changes that
become unnecessary as well, and I think the compiler will spot those
just fine.

[...]

> diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc

[...]

> @@ -7160,6 +7160,7 @@ build_reference_type(read_context&	ctxt,
>    result.reset(new reference_type_def(utype, is_lvalue, size,
>  				      /*alignment=*/0,
>  				      location()));
> +

Unnecessary white space change.

[...]

> diff --git a/src/abg-ir.cc b/src/abg-ir.cc
> index c279ad8..4aefc24 100644
> --- a/src/abg-ir.cc
> +++ b/src/abg-ir.cc
> @@ -212,7 +212,7 @@ location_manager::create_new_location(const std::string&	file_path,
>  /// Given an instance of location type, return the triplet
>  /// {path,line,column} that represents the source locus.  Note that
>  /// the location must have been previously created from the function
> -/// location_manager::expand_location otherwise this function yields
> +/// location_manager::create_new_location, otherwise this function yields

This is a legitimate change, but I think it is unrelated to the object
of this patch.  It should be a separate patchlet.  That patchlet is
obviously an obvious patch that I obviously pre-approve, should you
want to commit it yourself to master.  :-)

Other than that, as I said in preamble, this patch is OK to commit
once my comments are addressed.

Thanks!


-- 
		Dodji