[PATCH weston v5 08/11] drm: Code and comments reformatting for consistency with other backend configs

[Giulio Camuffo]

2016-04-13 16:17 GMT+03:00 Pekka Paalanen <ppaalanen at gmail.com>:
> On Wed, 13 Apr 2016 03:25:12 -0700
> Bryce Harrington <bryce at osg.samsung.com> wrote:
>
>> Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
>> ---
>>  src/compositor-drm.c | 10 ++++++----
>>  src/compositor-drm.h | 43 ++++++++++++++++++++++++++++---------------
>>  src/main.c           |  6 ++++--
>>  3 files changed, 38 insertions(+), 21 deletions(-)
>>
>
>> diff --git a/src/compositor-drm.h b/src/compositor-drm.h
>> index 423378a..3b54ca5 100644
>> --- a/src/compositor-drm.h
>> +++ b/src/compositor-drm.h
>> @@ -25,8 +25,8 @@
>>   * SOFTWARE.
>>   */
>>
>> -#ifndef WESTON_COMPOSITOR_DRM_H
>> -#define WESTON_COMPOSITOR_DRM_H
>> +#ifndef _WESTON_COMPOSITOR_DRM_H
>> +#define _WESTON_COMPOSITOR_DRM_H
>
> Hi,
>
> let's not add any more macros starting with an underscore. I suspect
> they may be reserved.

It does seem to be reserved indeed:
https://drj11.wordpress.com/2008/04/18/the-use-of-blah/
http://stackoverflow.com/questions/228783/what-are-the-rules-about-using-an-underscore-in-a-c-identifier
(this one is about c++ but since we allow the header to be included
from c++ with the extern "C" it makes sense to follow that rule)

>
>
>> diff --git a/src/main.c b/src/main.c
>> index 334464b..f652701 100644
>> --- a/src/main.c
>> +++ b/src/main.c
>> @@ -675,6 +675,7 @@ load_backend_new(struct weston_compositor *compositor, const char *backend,
>>  }
>>
>>
>> +// TODO: Why is there a wrapper around the drm config base object?
>
> Looks like a left-over: bad style and removed in a later patch.
>
>>  struct drm_config {
>>       struct weston_drm_backend_config base;
>>       bool use_current_mode;
>> @@ -731,7 +732,7 @@ load_drm_backend(struct weston_compositor *c, const char *backend,
>>       struct weston_config_section *section;
>>       int ret = 0;
>>
>> -     config = zalloc(sizeof *config);
>> +     config = zalloc(sizeof (struct drm_config));
>
> Please keep it as it was to ensure the variable and the sizeof
> obviously match. It removes the need for change here in the patch
> removing drm_config.
>
>>       if (!config)
>>               return -1;
>>
>> @@ -755,7 +756,8 @@ load_drm_backend(struct weston_compositor *c, const char *backend,
>>       config->base.base.struct_size = sizeof(struct weston_drm_backend_config);
>>       config->base.configure_output = drm_configure_output;
>>
>> -     if (load_backend_new(c, backend, &config->base.base) < 0) {
>> +     if (load_backend_new(c, backend,
>> +                          (struct weston_backend_config *)(&config->base)) < 0) {
>
> This I don't understand. Why? The old form is shorter, maintains
> type-safety, and does not depend on the particular layout of the struct.
>
>
>>               ret = -1;
>>               free(config->base.gbm_format);
>>               free(config->base.seat_id);
>
> All the above fixed, this gets:
> Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
>
>
> Thanks,
> pq
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>