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@mariadb.org