Re: [PATCH] initialize struct members

Stefan Sperling 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. Your log message looks much better.
I've changed it to your version.

Stefan

-- 
       ___
  oo  // \\      "De Chelonian Mobile"
 (_,\/ \_/ \     TortoiseSVN
   \ \_/_\_/>    The coolest Interface to (Sub)Version Control
   /_/   \_\     http://tortoisesvn.net