Nicolas.Williams at sun.com
Mon Aug 24 17:14:25 EDT 2009
On Mon, Aug 24, 2009 at 04:49:51PM -0400, Greg Hudson wrote:
> * Do not put parens around return expressions:
> - g_acquire_cred_imp_cred.c
> - g_acquire_cred_imp_name.c
> (These may be the result of copying existing code.)
Putting parens around return expressions is a Solaris C-style dictum.
If this code is based on Solaris' libgss, then I recommend adhering to
the Solaris C-style if possible, for now.
That applies to just a few of your other comments (e.g., put a space
between sizeof and the parenthesized type expression).
> * In krb5_gss_cred_id_rec you introduce some bitfields where none were
> present. I don't know if it is important to contain the size of id_rec
> structures, and using bitfields can expand the size of the code which
> processes those fields, which at times can be more undesirable than
> allocating a bit more memory in an infrequently-used structure. I am
> not sure how the balance comes out in this case, but please be aware of
> the tradeoff.
The *gss_*rec* structs are all private. Their size and layout can be
changed at any time without trouble.
I prefer bitfields where possible, and for several reasons:
- I wouldn't presume to be able to write more optimal code than the
compiler -- hard data is needed (and even then, there's more than one
- These flags are not referenced in performance sensitive fastpaths.
- Bitfields are clearer, more elegant, and easier to use that CPP
symbols and bit-wise expressions.
More information about the krbdev