maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Vicențiu!

On Aug 26, Vicențiu Ciorbaru wrote:
> Hi Sachim, Sergei!
> 
> One quick thing I wanted to point out. I did not specifically look at
> how things get called, but, when defining constants, I don't agree
> with:
> 
> > > +#define HA_HASH_STR_LEN             strlen(HA_HASH_STR)
> >
> Or:
> 
> > > +#define HA_HASH_STR_INDEX_LEN       strlen(HA_HASH_STR_INDEX)
> 
> This hides an underlying strlen. Better make it a real constant value.
> Perhaps the compiler is smart enough to optimize it away, but why risk it?

Right, we usually use sizeof() for this. With the pattern

  const LEX_CSTRING ha_hash_str= { STRING_WITH_LEN("HASH") };

although I'm pretty sure that any modern compiler will replace strlen("string")
with a constant.

> Another one is why not define them as const char * and const int? This also
> helps during debugging, as you can do:
> 
> (gdb) $ print HA_HAST_STR_INDEX_LEN

if you compile with -ggdb3, gdb will show macro values too :)

> I know that a lot of the code makes use of defines with #define, but
> why not enforce a bit of type safety while we're at it?

I don't disagree.
As far as this patch is concerned, I hope that in the final version there
will be no define for "HASH" or "HASH_INDEX" at all.
As a general rule, I agree that typed constants and sizeof() is
preferrable.

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx