[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Help-smalltalk] [PATCH v2 2/4] libgst: Miscellaneous improving to c
From: |
Lee Duhem |
Subject: |
Re: [Help-smalltalk] [PATCH v2 2/4] libgst: Miscellaneous improving to code style |
Date: |
Wed, 6 Dec 2017 22:29:39 +0800 |
Hello Holger,
On Sat, Dec 2, 2017 at 2:50 PM, Holger Freyther <address@hidden> wrote:
>
> > On 30. Nov 2017, at 17:31, Lee Duhem <address@hidden> wrote:
> >
>
> Hi again!
>
>
>
> > @@ -2025,7 +2023,7 @@ _gst_to_wide_cstring (OOP stringOOP)
> > string = (gst_unicode_string) OOP_TO_OBJ (stringOOP);
> > len = oop_num_fields (stringOOP);
> > result = (wchar_t *) xmalloc (len + 1);
> > - if (sizeof (wchar_t) == 4)
> > + if (sizeof (wchar_t) == sizeof(string->chars[0]))
>
> Okay. string->chars[0] is uint32_t so the above is identical but
> the code looks bad in general?
>
The code implies that when sizeof(string->chars[0]) equals 4, it can memcpy
from
string->chars to result. This change just wants to make that assumption
clear.
>
> len = oop_num_fields (stringOOP);
> result = (wchar_t *) xmalloc (len + 1);
>
>
> '12345678' asUnicodeString basicSize
> => 8
>
> There is barely any C API using "wchar_t" so it is no surprise
> the code might be broken. I think it needs to be:
>
> result = (wchar_t *) xmalloc ((len + 1) * sizeof(*result));
>
> and then maybe size(*result) == sizeof(string->chars[0])
>
Yes, we need to xmalloc more spaces for the result.
BTW, adding an assertion to make sure sizeof(wchar_t) >=
sizeof(string->chars[0])
looks like a good idea to prevent possible overwritten.
>
>
>
> > - && (flags & (GST_REBUILD_IMAGE | GST_MAYBE_REBUILD_IMAGE)) == 0
> > + && !rebuild_image_flags
>
> I am flying right now but what is the GNU coding style? I have
> my phases with !flag or flag == 0. I would only change it if there
> is either a majority in our code or GNU coding style has a
> statement about it.
>
I replaced that complicated condition with !rebuild_image_flags because we
already have that
variable, and it seems a good idea for me to simplify it.
About !flag or flag == 0, both work for me. I chose !flag because there
are already several examples
in the same source code file.
Regards,
lee
>
> cheers
> holger
>