I'll volunteer to take on this review, since I have some experience
with __attribute__(malloc) due to my work on gperftools.
First, while I disagree that the cost to the header file is basically
0 -- I think it definitely has a negative impact on readability, at
least to some extent -- I support Nuno's argument that it's an
optimization work making. At least in theory, the compiler can emit
faster code when it has these hints. Of course, it's testable to see
what effect this has in real life, and I'd be interested to see those
numbers. Do we have a performance test suite for pcre?
The code itself looks good, with a few comments:
} +#if defined(__GNUC__) && __GNUC__ >= 3
} +# define PCRE_ATTR_MALLOC __attribute__((__malloc__))
I prefer to check for __attribute__ support in autoconf, and condition
on #ifdef HAVE___ATTRIBUTE__ rather than checking for a particular
version of gcc. I guess in practice everyone who supports
__attribute__ also mimicks being gcc (does icc?) so maybe this is "6
of one, half-dozen of the other."
} +#if (defined(__GNUC__) && (__GNUC__ > 4 || (__GNUC__ == 4 &&
__GNUC_MINOR__ >= 3))) || __has_attribute(alloc_size)
Doesn't clang support __has_attribute(__alloc_size__) now? It seems
so from here:
http://clang.llvm.org/docs/LanguageExtensions.html#feature_check
Seems slightly better, to avoid the (admittedly small) chance an
application #defines alloc_size.
I'm not familiar with the alloc_size attribute; from this code it
sounds like it's fairly new. It would definitely be a good addition
to gperftools!, but I wonder how much benefit it gives to pcre. It
sounds like it only affects glibc's alloc_size() (?) which I doubt
anyone will use with these functions. If there was worry about the
readability of this header file, as Zoltan's comments indicate, one
possibility would be to keep attribute(__malloc__) but drop the
alloc_size stuff.
Overall, thanks for suggesting these improvements!
craig
