Yeah, that's probably picking nits, and about a relatively minor
change. ie diminishing returns to change
On Sep 10, 2008, at 16:47, Stefan Sperling <stsp_at_elego.de> wrote:
> On Wed, Sep 10, 2008 at 09:11:43PM +0200, Stefan Kščng wrote:
>> Stefan Sperling wrote:
>>> On Wed, Sep 10, 2008 at 08:01:12PM +0200, Lieven Govaerts wrote:
>>>> Stefan Kščng wrote:
>>>>> Hi,
>>>>>
>>>>> Before I commit this, maybe someone likes to first review it:
>>>>> this patch initializes all struct members in the functions
>>>>> svn_wc_conflict_description_create_text() and
>>>>> svn_wc_conflict_description_create_prop().
>>>>> Found this due to crashes in TSVN - it tried to access the
>>>>> merged_file
>>>>> member which had a non-NULL value but pointed to bogus memory.
>>>>>
>>>>> If nobody objects, I'll commit this in about an hour (shouldn't
>>>>> be much
>>>>> to object to IMHO).
>>>>>
>>>> Using apr_pcalloc has the same effect, but slightly easier for
>>>> the eyes.
>>>
>>> Oops, good point, actually.
>>>
>>> Stefan (you, not me :), do you want to make a follow-up commit that
>>> uses apr_pcalloc() instead?
>>
>> See r33019.
>
> Thanks!
>
>> Hope the commit message is ok.
>
> I took a look (because you asked).
>
> For reference, the log message:
>
> [[[
> Follow-up to r33017:
>
> Use apr_pcalloc to initialize svn_wc_conflict_description_t factory
> functions.
>
> * subversion/libsvn_wc/util.c
> (svn_wc_conflict_description_create_text),
> (svn_wc_conflict_description_create_prop): Instead of setting not
> relevant members to NULL, use apr_pcalloc instead of apr_palloc
> to get
> the memory for the structure.
> ]]]
>
> The first sentence looks wrong. It's not the functions themselves
> that are being initialised. Rather, the objects the functions are
> allocating are initialised.
>
> Also, the nested "instead of" looks a bit confusing to me (but
> probably just me).
>
> FWIW, I'd suggest something like:
>
> [[[
> Follow-up to r33017:
>
> * subversion/libsvn_wc/util.c
> (svn_wc_conflict_description_create_text),
> (svn_wc_conflict_description_create_prop): Use apr_pcalloc instead
> of apr_palloc to allocate memory. This way, we don't need to set
> any members to NULL manually.
> ]]]
>
> That said, log messages are a matter of taste and writing style,
> so you'd probably get entirely different suggestions from everyone
> else :)
>
> Thanks,
> Stefan
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
> For additional commands, e-mail: dev-help_at_subversion.tigris.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-09-11 00:01:12 CEST