Re: [PATCH 06/20] Integrate new symtab reader into corpus and read_context

[Dodji Seketeli <dodji@seketeli.org>]

Hello,

Matthias Maennich <maennich@google.com> a écrit:

> While reading the corpus in the read_context, also load the new type
> symtab object side-by-side and set it accordingly in the resulting
> corpus. This is still side by side and passive code that gets active in
> the following changes. This is applicable for the dwarf reader as well
> as for the reader that consumes XML.

I see, this makes sense.

[...]

> diff --git a/include/abg-fwd.h b/include/abg-fwd.h
> index bb839fe76315..76df1b51dae7 100644
> --- a/include/abg-fwd.h
> +++ b/include/abg-fwd.h
> @@ -1375,6 +1375,14 @@ typedef vector<suppression_sptr> suppressions_type;
>  
>  } // end namespace suppr
>  
> +namespace symtab_reader
> +{
> +
> +class symtab;
> +typedef std::shared_ptr<symtab> symtab_sptr;

We'd need a small comment here so that this shows up well in the API
doc.

[...]

> +++ b/src/abg-corpus.cc
> @@ -23,6 +23,7 @@ ABG_BEGIN_EXPORT_DECLARATIONS
>  #include "abg-ir.h"
>  #include "abg-reader.h"
>  #include "abg-sptr-utils.h"
> +#include "abg-symtab-reader.h"
>  #include "abg-tools-utils.h"
>  #include "abg-writer.h"
>  
> @@ -850,6 +851,14 @@ corpus::operator==(const corpus& other) const
>  	  && j == other.get_translation_units().end());
>  }
>  
> +void
> +corpus::set_symtab(symtab_reader::symtab_sptr symtab)
> +{priv_->symtab_ = symtab;}
> +
> +const symtab_reader::symtab_sptr&
> +corpus::get_symtab() const
> +{ return priv_->symtab_; }
> +

Likewise for these two function definitions.

[...]

> --- a/src/abg-dwarf-reader.cc
> +++ b/src/abg-dwarf-reader.cc

[...]

> +  const symtab_reader::symtab_sptr&
> +  symtab() const
> +  {

Likewise.

> +    if (!symtab_)
> +      symtab_ = symtab_reader::symtab::load(
> +	elf_handle(), options_.env, [&](const elf_symbol_sptr& symbol) {
> +	  return is_elf_symbol_suppressed(symbol);
> +	});

Throughout the code, we block braces always go on the new line.  So
let's try to do the same for lambdas, e.g:

  symtab_ =
    symtab_reader::symtab::load
    (elf_handle(), options_.env,
     [&](const elf_symbol_sptr& symbol)
     {
       return is_elf_symbol_suppressed(symbol);
     });

I guess I should update the coding standard guidelines with the case for
lambdas, etc, at some point.

[...]

> diff --git a/tests/test-symtab.cc b/tests/test-symtab.cc
> index ffb20a4dec44..ff467d8ad345 100644
> --- a/tests/test-symtab.cc
> +++ b/tests/test-symtab.cc

[...]

> -  REQUIRE(status
> -	  == (dwarf_reader::STATUS_OK
> -	      | dwarf_reader::STATUS_DEBUG_INFO_NOT_FOUND));
> +  REQUIRE(
> +    status
> +    == (dwarf_reader::STATUS_OK | dwarf_reader::STATUS_DEBUG_INFO_NOT_FOUND));

I think this change is useless.

>  }
>  

[...]

>
> 	* include/abg-corpus.h (corpus::set_symtab): New method declaration.
> 	  (corpus::get_symtab): New method declaration.
> 	* include/abg-fwd.h (symtab_reader::symtab_sptr): New forward
> 	  declaration.
> 	* src/abg-corpus-priv.h (corpus::priv::symtab_): New data member.
> 	* src/abg-corpus.cc
> 	  (corpus::set_symtab): Likewise.
> 	  (corpus::get_symtab): Likewise.
> 	* src/abg-dwarf-reader.cc (read_context::symtab_): New data member.
> 	  (read_context::initialize): reset symtab_ as well
> 	  (read_context::symtab): new method that loads a symtab on
> 	  first access and returns it.
> 	  (read_debug_info_into_corpus): also set the new symtab object
> 	  on the current corpus.
> 	  (read_corpus_from_elf): Also determine (i.e. load) the new
> 	  symtab object and contribute to the load status.
> 	* src/abg-reader.cc (read_corpus_from_input): also set the new
> 	  type symtab when reading from xml.
> 	* tests/test-symtab.cc: Add test assertions.
>
> Reviewed-by: Giuliano Procida <gprocida@google.com>
> Signed-off-by: Matthias Maennich <maennich@google.com>

This is OK to go in with the changes stated above and once the previous
patch is applied.

Thanks!

Cheers,

-- 
		Dodji