[PATCH 1/2] abg-writer: Add support for stable hash type ids.

[Dodji Seketeli]

Hello,

Giuliano Procida <gprocida@google.com> a écrit:

[...]

> diff --git a/include/abg-writer.h b/include/abg-writer.h
> index 8086d828..021a95b7 100644
> --- a/include/abg-writer.h
> +++ b/include/abg-writer.h
> @@ -38,6 +38,12 @@ namespace xml_writer
>  
>  using namespace abigail::ir;
>  
> +enum type_id_style_kind

I think this enum could use some comment that should start with '///',
so that it shows up in the doxygen-generated API doc.

> +{
> +  COUNTER,

I'd rather name this something more self-documented like
COUNTER_BASED_TYPE_ID_STYLE ...

> +  HASH

... and this one something like HASH_BASED_TYPE_ID_STYLE.

> +};
> +

[...]

> +++ b/src/abg-writer.cc
> @@ -27,6 +27,7 @@
>  
>  #include "config.h"
>  #include <assert.h>
> +#include <iomanip>
>  #include <iostream>
>  #include <fstream>
>  #include <sstream>
> @@ -56,6 +57,39 @@ ABG_BEGIN_EXPORT_DECLARATIONS
>  ABG_END_EXPORT_DECLARATIONS
>  // </headers defining libabigail's API>
>  
> +namespace
> +{
> +/// Compute a stable string hash.
> +///
> +/// std::hash has no portability or stability guarantees so is
> +/// unsuitable where reproducibility is a requirement.
> +///
> +/// This is the 32-bit FNV-1a algorithm. The algorithm, reference code
> +/// and constants are all unencumbered. It is fast and has reasonable
> +/// distribution properties.
> +///
> +/// https://en.wikipedia.org/wiki/Fowler-Noll-Vo_hash_function
> +///
> +/// @param str the string to hash.
> +///
> +/// @return an unsigned 32 bit hash value.
> +uint32_t
> +stable_hash(const std::string& str)

The file include/abg-hash.h and src/abg-hash.cc exist to host
implementations of hashing facilities.  I would put this function in
there.

Also, please rename it to an explicit name like maybe fnv_hash.

> +{
> +  const uint32_t prime = 0x01000193;
> +  const uint32_t offset_basis = 0x811c9dc5;
> +  uint32_t hash = offset_basis;
> +  for (std::string::const_iterator i = str.begin(); i != str.end(); ++i)
> +    {
> +      uint8_t byte = *i;
> +      hash = hash ^ byte;
> +      hash = hash * prime;
> +    }
> +  return hash;
> +}
> +
> +} // namespace
> +
>  namespace abigail
>  {
>  using std::cerr;
> @@ -177,7 +211,9 @@ class write_context
>    bool					m_write_parameter_names;
>    bool					m_short_locs;
>    bool					m_write_default_sizes;
> +  type_id_style_kind			m_type_id_style;
>    mutable type_ptr_map			m_type_id_map;
> +  mutable std::unordered_set<uint32_t>	m_used_type_id_hashes;

In our current setting (before c++11), unordered_set is part of the
std::tr1 namespace, stricto sensu.  Thanks to the magic in place, just
using unordered_set rather than std::unordered_set should do the trick.
Otherwise this won't compile on a compiler that doesn't support c++11.

Also, I think you should just conflate the testing patch that follows up
with this one.

All in all, for what it's worth, I like this patch very much!  Thank you
for working on it.

Cheers,

-- 
		Dodji