-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 09/08/2009 08:41 AM, Dmitri Pal wrote: > Thank you for review, comments inline. > > Stephen Gallagher wrote: >> On 09/07/2009 10:05 AM, Jakub Hrozek wrote: >>> On 09/05/2009 02:32 AM, Dmitri Pal wrote: >>>> See patch comment. >>>> This patch most likely will not apply cleanly without my previous ELAPI >>>> patches. >>>> So far it is 4 th pending patch. >>>> More to come. >>> In general, I think the patch is OK. Some comments, though: >> >>> * col_copy_traverse_handler got pretty long, one function now accounts >>> for more that 200 lines doing different things, esp. with different copy >>> mode handling. Maybe it would be more readable if it was split into >>> smaller ones? > > It got a bit long, I agree but splitting will IMO break the internal > line of thought related to this function. > I took a look at it and could not see how to better slice it. > Allowing longer lines would have made it twice shorter. :-) >> >>> * the copy mode could be made unsigned (or even enum copy_mode {...}), >>> you wouldn't have to test for mode<0 > > I wanted to be consistent with other modes in similar functions that are > integers. > May be they all should have been unsigned but I would not go for a > sweeping change at least now. > Fill file a bug though: #168 > >> >>> * if you really need assigning const* to non-const*, can you just use >>> something like the discard_const_p macro in server/util/util.h, I think >>> it's way better that memcpy-ing the memory >> >>> Jakub >> >> I think I prefer Dmitri's approach here, actually. name_to_use is not >> under Dmitri's control (unless I'm misreading the patch), so it's >> theoretically possible for the owner of that memory to change its >> contents underneath you. If you're going to need that data to remain >> constant and trustworthy further on in the code (e.g. used for >> comparisons), you need to copy its current state. > Steve I do not think I get your statement. I copy the pointer instead > of direct assignment since any attempt with direct assignment > was producing warnings. > > Here is the situation: > The internal function is recursive. The pointer I use need to be > initialized to a value. > During the recursion the state changes and the pointer points to > different strings > To make this happen I had to event something. And I hate warnings. > So I came with the implemented approach and put a comment there to explain. > > Jakub I also looked at the macro in util.h. > I tried similar approaches but all of them caused different kinds of > warnings. > I am not sure that this typecast will help. At least I do not see how > this typecast > is different from any other typecast. May be there is something specific > about uintptr_t? > > I am unclear about couple things here: > a) Is my solution acceptable for the given situation? > b) If not what is the preferred solution? > c) What is different about the given typecast and why this typecast > suppresses > warning (if it does) while others do not? > > Please advise.
I think you misunderstood what I was saying. I think you did it the right way, Dmitri. (provided of course that you free that memory responsibly when you no longer need it) > >> _______________________________________________ >> sssd-devel mailing list >> sssd-devel@lists.fedorahosted.org >> https://fedorahosted.org/mailman/listinfo/sssd-devel >> > _______________________________________________ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://fedorahosted.org/mailman/listinfo/sssd-devel > - -- 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/ iEYEARECAAYFAkqmVBIACgkQeiVVYja6o6PUXgCaAgiRRC6DEr01Ozeex16GFFmh a9oAoK9+jn43SG/MEnW+MdS57XGq9txO =WpCM -----END PGP SIGNATURE----- _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel