[Bf-committers] [Bf-blender-cvs] SVN commit: /data/svn/bf-blender [54258] trunk/blender/source/blender/ blenloader/intern/readfile.c: add missing break in direct_link_constraints, CONSTRAINT_SPACEONCE flag was getting set to CONSTRAINT_TYPE_KINEMATIC.

[Campbell Barton]

On Sat, Feb 2, 2013 at 4:09 PM, Joshua Leung <aligorith at gmail.com> wrote:
> +1 for the fix
>
> BUT
> -1 for the trojan horsed style cleanup.
>
> break;  statements here are part of the switch-case structure. By lumping
> them inside these blocks, you run a greater risk of people glossing over
> them or even leaving them out by mistake, as they effectively look like
> just any instruction there.
>
> A particularly ugly (and confusing) example from a few thousand commits ago
> ended up looking like
>    case ABC:
>    {
>        ...
>        if (...) {
>             break;
>        }
>        break;
>     }
>     case DEF:
>     ...
>
> Not indenting them while leaving them outside the block, i.e.
>     case ABC:
>     {
>         ... some code here ...
>     }
>     break;
>     case ...
> Again raises this gloss-over risk - here break+case may be mistaken for
> case+case (as many if not most IDE's I've seen usually highlight these in
> the same way).
>
>
> On Sat, Feb 2, 2013 at 5:13 PM, Campbell Barton <ideasman42 at gmail.com>wrote:
>
>> Revision: 54258
>>
>> http://projects.blender.org/scm/viewvc.php?view=rev&root=bf-blender&revision=54258
>> Author:   campbellbarton
>> Date:     2013-02-02 04:13:38 +0000 (Sat, 02 Feb 2013)
>> Log Message:
>> -----------
>> add missing break in direct_link_constraints, CONSTRAINT_SPACEONCE flag
>> was getting set to CONSTRAINT_TYPE_KINEMATIC.
>>
>> Modified Paths:
>> --------------
>>     trunk/blender/source/blender/blenloader/intern/readfile.c
>>
>> Modified: trunk/blender/source/blender/blenloader/intern/readfile.c
>> ===================================================================
>> --- trunk/blender/source/blender/blenloader/intern/readfile.c   2013-02-02
>> 02:42:12 UTC (rev 54257)
>> +++ trunk/blender/source/blender/blenloader/intern/readfile.c   2013-02-02
>> 04:13:38 UTC (rev 54258)
>> @@ -2679,15 +2679,15 @@
>>                                 data->prop = newdataadr(fd, data->prop);
>>                                 if (data->prop)
>>                                         IDP_DirectLinkProperty(data->prop,
>> (fd->flags & FD_FLAGS_SWITCH_ENDIAN), fd);
>> +                               break;
>>                         }
>> -                               break;
>>                         case CONSTRAINT_TYPE_SPLINEIK:
>>                         {
>>                                 bSplineIKConstraint *data= con->data;
>> -
>> +
>>                                 data->points= newdataadr(fd, data->points);
>> +                               break;
>>                         }
>> -                               break;
>>                         case CONSTRAINT_TYPE_KINEMATIC:
>>                         {
>>                                 bKinematicConstraint *data = con->data;
>> @@ -2697,14 +2697,15 @@
>>
>>                                 /* version patch for runtime flag, was not
>> cleared in some case */
>>                                 data->flag &= ~CONSTRAINT_IK_AUTO;
>> +                               break;
>>                         }
>>                         case CONSTRAINT_TYPE_CHILDOF:
>>                         {
>>                                 /* XXX version patch, in older code this
>> flag wasn't always set, and is inherent to type */
>>                                 if (con->ownspace == CONSTRAINT_SPACE_POSE)
>>                                         con->flag |= CONSTRAINT_SPACEONCE;
>> +                               break;
>>                         }
>> -                               break;
>>                 }
>>         }
>>  }

I was under the impression that it was more common in Blender to use
breaks within the braces, but checking again we don't have a
convention for this, each dev has been doing it differently.

Though I now see all your code uses them outside, other areas of
blender are inconsistent.

I don't have a strong opinion here tho I would prefer this stays
consistent per file at least. In readfile.c lines 3588, 8602 do this.
Line 1946 not.

As for devs moving break's into nested braces, when editing flow
control with case statements you just need to take care and not gloss
over!, but OK - it happens sometimes so you have a point :S.

Would be good to agree on one and note in:
http://wiki.blender.org/index.php/Dev:Doc/CodeStyle