maria-developers team mailing list archive

[Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx>]

Davi Arnaut <davi@xxxxxxxxxxx> writes:

> On Wed, Aug 1, 2012 at 12:46 AM, Kristian Nielsen
> <knielsen@xxxxxxxxxxxxxxx> wrote:
>> Davi Arnaut <davi@xxxxxxxxxxx> writes:
>>
>>> On Thu, Jul 26, 2012 at 5:50 PM, Igor Babaev <igor@xxxxxxxxxxxx> wrote:
>>>> +  bzero(column_stats, sizeof(Column_statistics) * cnt);
>>>
>>> That's not allowed for non-POD types.
>>
>> Can you elaborate on exactly when bzero() (or memset() or similar) is allowed
>> or not allowed?

Ok, so I got time to research this a bit more.

The class in question is like this, the original has more members but this is
the essential structure:

    class Column_statistics
    {
    private:
      static const uint Scale_factor_nulls_ratio= 100000;
    public:
      Field *min_value; 
    private:
      ulong nulls_ratio;
    public:
      void set_all_nulls() { ... }
    };

The question is whether it is ok to allocate this with alloc_root() or
similar, and initialise it with bzero(pointer, sizeof(Column_statistics)).
(The issue of bzero vs. memset is a separate one, not discussed in this mail).

Davi points out that this is not a POD. This is because a POD must be a C++
aggregate, and aggregates can not have private members.

Unfortunately, it seems the question of whether bzero is allowed for a
non-POD, is more difficult to answer.

If we look to the standards, the answer is no. It is not well-defined to
bzero() any data except integers. This is what the standard says. However,
this is unhelpful for the case at hand, as bzero to initialise a simple struct
is a well-established practice in systems programming and commonly accepted to
be valid.

It seems generally accepted that bzero() of a POD is acceptable. Since the
only thing that makes Column_statistics not a POD is the presence of private:
members, the question becomes: Should bzero initialisation of classes with
private members be disallowed?

I could not find any definitive reference to support such disallowance.

I am not sure why private members would be disallowed in aggretate, and hence
POD. My guess is because aggregates may be initialised with an initialiser
list, which makes no sense for private member. Or maybe compilers could
re-arrance private members to be placed after public members. Or compilers
could optimise based on knowledge that no external code can modify a private
member (so it does not need to be spilled over a call to pthread_mutex_lock()
or similar). But none of this would give a problem with bzero-initialisation
after alloc.

So I do not know of a language standard/compiler argument to disallow
bzero-initialisation of non-POD.

----

But I would still strongly advice to be very careful with such code, and I
think Davi's original concern is similar to mine. The problem is that
Column_statistics already looks a lot like a nicely encapsulated proper C++
class. It has private members, accessors, it even defines some constants. One
could easily imagine that in the future, there will be the temptation to
extend it even more so, like add a virtual function - and then we suddenly
have a big problem. bzero() is most certainly _NOT_ ok for such class.

So I would personally stay well away from this grey area, and rather

 - _either_ use only a simple struct with no member functions or private or
   anything like that - and use bzero/memcpy and such.

 - _or_ do a full class, but then only initialise it with proper call of
   contructors (whether explicitly or implicitly defined).

That makes it much clearer whether a given struct class is one or the other.
But I suppose the optimiser code already heavily uses a different strategy.

At the least, the declaration of class Column_statistics should have a comment
saying that it is initialised with bzero(), so it can not use features not
compatible with this.

The nice thing about Davi's suggested rule "no bzero of non-POD" is that it is
simple, easy to remember, and easy to state. Which is the primary point of
coding style guidelines in the first place: make the rules clear, so there is
less wasted time discussing what they are.

Hope this helps,

 - Kristian.