[SSSD] Coding standard question (was ELAPI Adding file... )

Stephen Gallagher sgallagh at redhat.com
Fri Sep 4 16:53:22 UTC 2009


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/04/2009 12:38 PM, Dmitri Pal wrote:
> Stephen Gallagher wrote:
>> On 09/02/2009 08:27 PM, Dmitri Pal wrote:
>>> Hi,
>>
>>> This patch is next significant chunk of code.
>>> See patch comments for the main description.
>>> It requires previous patch that has been reviewed but was not pushed
>> yet.
>>> It addresses some of the suggestions risen during the review.
>>> I also added couple tickets: to eventually remove all the "Fixme"
>>> comments and to refactor places where I allocated context structs and
>>> initialize them.
>>
>>> Checked with make check, valgrind and did in tree and parallel builds.
>>> All works, no leaks.
>>
>>> Sorry that it is too big.
>>> I need to move on and there are many things that are still missing.
>>> I will start working on  the next set of the functionality tomorrow
>>> mainly adding async processing. That would definitely affect some of the
>>> arguments passed all along so expect changes to many modules.
>>
>>
>>
>>> ------------------------------------------------------------------------
>>
>>> _______________________________________________
>>> sssd-devel mailing list
>>> sssd-devel at lists.fedorahosted.org
>>> https://fedorahosted.org/mailman/listinfo/sssd-devel
>>
>> Nack.
>>
>> Please do not use a #define when you really mean "typedef"
>>
>> you probably wanted
>> typedef struct elapi_data_out {
>>     char *buffer;
>>     uint32_t size;
>>     uint32_t length;
>>     uint32_t written;
>> } SEDO;
>>
>> But I still don't much care for SEDO. But this is an internal-only
>> header so I'm not going to complain too loudly. That said, we generally
>> frown upon using typedef anywhere but function pointers, since they
>> obfuscate the actual type you're using. I'd much prefer if you used
>> "struct elapi_data_out" written out fully.
>>
>> This is the only problem I see in a high-level overview of this code.
>> Jakub was going to do a more in-depth review, so I'll wait on his
>> comments as well.
>>
> Hm, I was testing you :-)
> I did not do the typedef exactly because the of the policy you mentioned.
> Simo mentioned during the coding style discussions that he would prefer
> defines to typedes since the debugger in such cases works correctly
> and shows the contents of the structure while the typedef
> obfuscates things.
> The spelled out struct made the lines pretty long so I tried
> to shorten it. And then it occurred to me that I might just
> use a define.
> The only thing that is not correct in your assessment
> is that it is completely internal. It is internal and low level but
> will be a part of the helper suit that the developers of the
> externally loadable ELAPI providers would have to use
> so it will be exposed.
> Keeping this in mind I want an answer to a more general
> question: do we think such #defines are acceptable or not.
> 
> If not I will  file a ticket to clean it up as a separate patch.
> If it is Ok I will add it to the standard.
> Please chime in.
>   
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel
> 


My personal preference is to not use typedefs OR defines, and instead
rely on using the 'struct <name>' approach.

Aliasing variable types is always confusing to someone reading the code.
The only exception I feel is warranted is typedef-ing a function pointer
prototype, since otherwise they are just too long to be reasonably used
throughout the code.

- -- 
Stephen Gallagher
RHCE 804006346421761

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkqhRf4ACgkQeiVVYja6o6P6jQCfV0wJmluh33KirHc0dg2a6+C0
odUAnjQvP//sQlJBhcFq5veWeb/h2NJW
=XfGk
-----END PGP SIGNATURE-----



More information about the sssd-devel mailing list