Re: [waffle] [PATCH 05/10] waffle: remove found_platform from waffle_init_parse_attrib_list

2014-06-10 Thread Chad Versace 
On Fri, Jun 06, 2014 at 12:24:06PM +0100, Emil Velikov wrote:
 On 06/06/14 07:25, Chad Versace wrote:
  On Sat, May 31, 2014 at 03:22:03AM +0100, Emil Velikov wrote:
  Whenever a platform is missing a case statement, the default will
  kick in throwing an error and exiting the function.
  
  Ah, but that's not what 'found_platform' is checking for...
  
  -if (!found_platform) {
  -wcore_errorf(WAFFLE_ERROR_BAD_ATTRIBUTE,
  - attribute list is missing WAFFLE_PLATFORM);
  -return false;
  -}
  
  ... waffle_init() uses 'found_platform' to check if the user provided
  WAFFLE_PLATFORM.
  
 AFAICS that is handled by the default switch case.
 
  I admit that waffle_init_parse_attrib_list() is a clumsily written
  function. It was one of the very first functions in Waffle's codebase.
  
 IMHO the code bails out if we've missed (partially or fully) any platform
 and/or if the user has provided a invalid WAFFLE_PLATFORM. Thus we will reach
 the if (!found_platform)... only when the variable is already set (via the
 CASE_DEFINED_PLATFORM macro).
 
 Perhaps this code is targeting some elaborate use-case which I'm failing to
 see here?

I think you're failing to see the use-case because it's extremely
*unelaborate*.

This block...

if (!found_platform) {
wcore_errorf(WAFFLE_ERROR_BAD_ATTRIBUTE,
 attribute list is missing WAFFLE_PLATFORM);
return false;
}

... catches the case when the user supplies *no* attribute. That is, it
catches these three erroneous cases:

waffle_init(NULL);
waffle_init((int32_t[]){});
waffle_init((int32_t[]){0});

In other words, 'found_platform' detects when codeflow altogether skips
the switch statement.
___
waffle mailing list
waffle@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/waffle

Re: [waffle] [PATCH 05/10] waffle: remove found_platform from waffle_init_parse_attrib_list

2014-06-06 Thread Chad Versace 
On Sat, May 31, 2014 at 03:22:03AM +0100, Emil Velikov wrote:
 Whenever a platform is missing a case statement, the default will
 kick in throwing an error and exiting the function.

Ah, but that's not what 'found_platform' is checking for...

 -if (!found_platform) {
 -wcore_errorf(WAFFLE_ERROR_BAD_ATTRIBUTE,
 - attribute list is missing WAFFLE_PLATFORM);
 -return false;
 -}

... waffle_init() uses 'found_platform' to check if the user provided
WAFFLE_PLATFORM.

I admit that waffle_init_parse_attrib_list() is a clumsily written
function. It was one of the very first functions in Waffle's codebase.
___
waffle mailing list
waffle@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/waffle

Re: [waffle] [PATCH 05/10] waffle: remove found_platform from waffle_init_parse_attrib_list

2014-06-06 Thread Emil Velikov 
On 06/06/14 07:25, Chad Versace wrote:
 On Sat, May 31, 2014 at 03:22:03AM +0100, Emil Velikov wrote:
 Whenever a platform is missing a case statement, the default will
 kick in throwing an error and exiting the function.
 
 Ah, but that's not what 'found_platform' is checking for...
 
 -if (!found_platform) {
 -wcore_errorf(WAFFLE_ERROR_BAD_ATTRIBUTE,
 - attribute list is missing WAFFLE_PLATFORM);
 -return false;
 -}
 
 ... waffle_init() uses 'found_platform' to check if the user provided
 WAFFLE_PLATFORM.
 
AFAICS that is handled by the default switch case.

 I admit that waffle_init_parse_attrib_list() is a clumsily written
 function. It was one of the very first functions in Waffle's codebase.
 
IMHO the code bails out if we've missed (partially or fully) any platform
and/or if the user has provided a invalid WAFFLE_PLATFORM. Thus we will reach
the if (!found_platform)... only when the variable is already set (via the
CASE_DEFINED_PLATFORM macro).

Perhaps this code is targeting some elaborate use-case which I'm failing to
see here?

-Emil
___
waffle mailing list
waffle@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/waffle