[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: Triggers status?



Raphael Hertzog writes ("Re: Triggers status?"):
> * you replaced a bunch of "NULL" by "(char*)0". This reverts 
>   http://git.debian.org/?p=dpkg/dpkg.git;a=commit;h=4e5846ccd3dcc33504aba8ef35a8962bccfd562e
>   I believe you should use NULL everywhere. (My personal taste also favors
>   NULL over "(char*)0" but it's not even a question of what I prefer since
>   I'm not an official dpkg maintainer)

This isn't a matter of preference, I'm afraid.  I reverted this
because the change was wrong.  NULL is incorrect in that context (a
stdarg function expecting a char*), because it may be #define'd to 0.

I had to make a decision about it since the (char*)0 -> NULL change
was made after I started my branch, so at some point when I merged it
resulted in an edit conflict.

> * it would have been nice to have the various bits of refactoring somewhat
>   separated (in other commits I mean) to simplify the review (it's still
>   doable with "git rebase -i", selecting "edit" on commits to split, and
>   then using "git add -i" to add subset of changes to the index and to
>   commit them in several steps).
>   I say this for the future but it's not a pre-requesite for further
>   review (at least not on my part).

I can see that that would be helpful but in practice doing things that
way makes the coding a lot harder.  Normally, for a smallish change, I
would definitely try to make separate patches for anything else I
stumbled across.  Likewise if I thought there was a serious
possibility that some of my changes would need to remain as a separate
branch for a long time while others are merged into the trunk.

But when one's doing the kind of substantial engineering required in
this case, the extra mindspace needed to retain the different
branches, merge the bugfixes in the right way, and so forth, is often
better spent on remembering how all of the code and semantics fitted
together.

> Direct patch review:
...
> +"  [--no-]triggers            Skip or force consequential trigger processing.\n"
> Should logically be "--[no-]triggers".

Well spotted; fixed.

Ian.



Reply to: