Re: svn commit: r322041 - head/sys/kern

[Warner Losh]

On Fri, Aug 4, 2017 at 9:22 PM, Bruce Evans  wrote:

> On Fri, 4 Aug 2017, Alan Cox wrote:
>
> Log:
>>  In case readers are misled by expressions that combine multiplication and
>>  division, add parentheses to make the precedence explicit.
>>
>>  Submitted by:  Doug Moore 
>>  Requested by:  imp
>>  Reviewed by:   imp
>>  MFC after: 1 week
>>  X-MFC after:   r321840
>>  Differential Revision: https://reviews.freebsd.org/D11815
>>
>
> This obfuscates the necessary parentheses.
>
> Modified: head/sys/kern/subr_blist.c
>> ...
>> static inline daddr_t
>> radix_to_skip(daddr_t radix)
>> {
>>
>> return (radix /
>> -   (BLIST_BMAP_RADIX / BLIST_META_RADIX * (BLIST_META_RADIX -
>> 1)));
>> +   ((BLIST_BMAP_RADIX / BLIST_META_RADIX) * (BLIST_META_RADIX -
>> 1)));
>> }
>>
>
> Readers now have to do a more complete parse to find the interesting parts,
> and writers have to count to a large number to get the count right when
> the parantheses pile up at the right.
>
> This expression is relatively simple to parse to remove the obfuscation,
> but consider more complicated cases:
>
> (1)
> (a + b + c + d + e) + (f + g + h + i + j)
>
> in floating point so that addition is not associative and the order
> matters.
> The order is left to right in C, and this expression uses 2 sets of
> parentheses to not use left to right for all terms.  Full parentheses gives
> the good obfuscation:
>
> a + b) + c) + d) + e) + f + g) + h) + i) + j)
>
> (2) Similarly with +- instead of all +.  The order matters much more, but I
> don't remember ever seeing expressions with only +- being obfuscated by
> parentheses, except in floating point code where the author wants to
> emphasize the left to right evaluation.  I guess there are also examples
> with integer types.  Even with all + operations, the order is critical
> with plain ints, since different orders might cause overflow, and with
> mixed signed/unsigned/small/large integer types, the promotions depend
> on the order.
>
> (3) Similarly with */ instead of +-.  These are even more similar in
> programming uses than in math structures, since + is always associative
> and commutative in math structures, but it is not even commutative in
> programming.


You know, for this case, it's totally cool.

Warner
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r322041 - head/sys/kern

[Bruce Evans]

On Fri, 4 Aug 2017, Alan Cox wrote:


Log:
 In case readers are misled by expressions that combine multiplication and
 division, add parentheses to make the precedence explicit.

 Submitted by:  Doug Moore 
 Requested by:  imp
 Reviewed by:   imp
 MFC after: 1 week
 X-MFC after:   r321840
 Differential Revision: https://reviews.freebsd.org/D11815


This obfuscates the necessary parentheses.


Modified: head/sys/kern/subr_blist.c
...
static inline daddr_t
radix_to_skip(daddr_t radix)
{

return (radix /
-   (BLIST_BMAP_RADIX / BLIST_META_RADIX * (BLIST_META_RADIX - 1)));
+   ((BLIST_BMAP_RADIX / BLIST_META_RADIX) * (BLIST_META_RADIX - 1)));
}


Readers now have to do a more complete parse to find the interesting parts,
and writers have to count to a large number to get the count right when
the parantheses pile up at the right.

This expression is relatively simple to parse to remove the obfuscation,
but consider more complicated cases:

(1)
(a + b + c + d + e) + (f + g + h + i + j)

in floating point so that addition is not associative and the order matters.
The order is left to right in C, and this expression uses 2 sets of
parentheses to not use left to right for all terms.  Full parentheses gives
the good obfuscation:

a + b) + c) + d) + e) + f + g) + h) + i) + j)

(2) Similarly with +- instead of all +.  The order matters much more, but I
don't remember ever seeing expressions with only +- being obfuscated by
parentheses, except in floating point code where the author wants to
emphasize the left to right evaluation.  I guess there are also examples
with integer types.  Even with all + operations, the order is critical
with plain ints, since different orders might cause overflow, and with
mixed signed/unsigned/small/large integer types, the promotions depend
on the order.

(3) Similarly with */ instead of +-.  These are even more similar in
programming uses than in math structures, since + is always associative
and commutative in math structures, but it is not even commutative in
programming.

Bruce
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r322041 - head/sys/kern

[Alan Cox]

On 08/04/2017 02:57, Oliver Pinter wrote:
>
>
> On Friday, August 4, 2017, Alan Cox  > wrote:
>
> Author: alc
> Date: Fri Aug  4 04:23:23 2017
> New Revision: 322041
> URL: https://svnweb.freebsd.org/changeset/base/322041
> 
>
> Log:
>   In case readers are misled by expressions that combine
> multiplication and
>   division, add parentheses to make the precedence explicit.
>
>   Submitted by: Doug Moore >
>   Requested by: imp
>   Reviewed by:  imp
>   MFC after:1 week
>   X-MFC after:  r321840
>   Differential Revision:https://reviews.freebsd.org/D11815
> 
>
> Modified:
>   head/sys/kern/subr_blist.c
>
> Modified: head/sys/kern/subr_blist.c
> 
> ==
> --- head/sys/kern/subr_blist.c  Fri Aug  4 04:20:11 2017   
> (r322040)
> +++ head/sys/kern/subr_blist.c  Fri Aug  4 04:23:23 2017   
> (r322041)
> @@ -110,6 +110,7 @@ __FBSDID("$FreeBSD$");
>  #definebitcount64(x)   __bitcount64((uint64_t)(x))
>  #define malloc(a,b,c)  calloc(a, 1)
>  #define free(a,b)  free(a)
> +#define CTASSERT(expr)
>
>
> Is this dummy define intended? 
>  

Yes, it is for user-space, stand-alone compilation of this file.

>
>  #include 
>
> @@ -142,6 +143,8 @@ static void blst_radix_print(blmeta_t *scan,
> daddr_t b
>  static MALLOC_DEFINE(M_SWAP, "SWAP", "Swap space");
>  #endif
>
> +CTASSERT(BLIST_BMAP_RADIX % BLIST_META_RADIX == 0);
> +
>  /*
>   * For a subtree that can represent the state of up to 'radix'
> blocks, the
>   * number of leaf nodes of the subtree is
> L=radix/BLIST_BMAP_RADIX.  If 'm'
> @@ -151,17 +154,19 @@ static MALLOC_DEFINE(M_SWAP, "SWAP", "Swap
> space");
>   * in the 'meta' functions that process subtrees.  Since integer
> division
>   * discards remainders, we can express this computation as
>   * skip = (m * m**h) / (m - 1)
> - * skip = (m * radix / BLIST_BMAP_RADIX) / (m - 1)
> - * and if m divides BLIST_BMAP_RADIX, we can simplify further to
> - * skip = radix / (BLIST_BMAP_RADIX / m * (m - 1))
> - * so that a simple integer division is enough for the calculation.
> + * skip = (m * (radix / BLIST_BMAP_RADIX)) / (m - 1)
> + * and since m divides BLIST_BMAP_RADIX, we can simplify further to
> + * skip = (radix / (BLIST_BMAP_RADIX / m)) / (m - 1)
> + * skip = radix / ((BLIST_BMAP_RADIX / m) * (m - 1))
> + * so that simple integer division by a constant can safely be
> used for the
> + * calculation.
>   */
>  static inline daddr_t
>  radix_to_skip(daddr_t radix)
>  {
>
> return (radix /
> -   (BLIST_BMAP_RADIX / BLIST_META_RADIX *
> (BLIST_META_RADIX - 1)));
> +   ((BLIST_BMAP_RADIX / BLIST_META_RADIX) *
> (BLIST_META_RADIX - 1)));
>  }
>
>  /*
> ___
> svn-src-h...@freebsd.org  mailing list
> https://lists.freebsd.org/mailman/listinfo/svn-src-head
> 
> To unsubscribe, send any mail to
> "svn-src-head-unsubscr...@freebsd.org "
>

___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r322041 - head/sys/kern

[Oliver Pinter]

On Friday, August 4, 2017, Alan Cox  wrote:

> Author: alc
> Date: Fri Aug  4 04:23:23 2017
> New Revision: 322041
> URL: https://svnweb.freebsd.org/changeset/base/322041
>
> Log:
>   In case readers are misled by expressions that combine multiplication and
>   division, add parentheses to make the precedence explicit.
>
>   Submitted by: Doug Moore >
>   Requested by: imp
>   Reviewed by:  imp
>   MFC after:1 week
>   X-MFC after:  r321840
>   Differential Revision:https://reviews.freebsd.org/D11815
>
> Modified:
>   head/sys/kern/subr_blist.c
>
> Modified: head/sys/kern/subr_blist.c
> 
> ==
> --- head/sys/kern/subr_blist.c  Fri Aug  4 04:20:11 2017(r322040)
> +++ head/sys/kern/subr_blist.c  Fri Aug  4 04:23:23 2017(r322041)
> @@ -110,6 +110,7 @@ __FBSDID("$FreeBSD$");
>  #definebitcount64(x)   __bitcount64((uint64_t)(x))
>  #define malloc(a,b,c)  calloc(a, 1)
>  #define free(a,b)  free(a)
> +#define CTASSERT(expr)


Is this dummy define intended?


>
>  #include 
>
> @@ -142,6 +143,8 @@ static void blst_radix_print(blmeta_t *scan, daddr_t b
>  static MALLOC_DEFINE(M_SWAP, "SWAP", "Swap space");
>  #endif
>
> +CTASSERT(BLIST_BMAP_RADIX % BLIST_META_RADIX == 0);
> +
>  /*
>   * For a subtree that can represent the state of up to 'radix' blocks, the
>   * number of leaf nodes of the subtree is L=radix/BLIST_BMAP_RADIX.  If
> 'm'
> @@ -151,17 +154,19 @@ static MALLOC_DEFINE(M_SWAP, "SWAP", "Swap space");
>   * in the 'meta' functions that process subtrees.  Since integer division
>   * discards remainders, we can express this computation as
>   * skip = (m * m**h) / (m - 1)
> - * skip = (m * radix / BLIST_BMAP_RADIX) / (m - 1)
> - * and if m divides BLIST_BMAP_RADIX, we can simplify further to
> - * skip = radix / (BLIST_BMAP_RADIX / m * (m - 1))
> - * so that a simple integer division is enough for the calculation.
> + * skip = (m * (radix / BLIST_BMAP_RADIX)) / (m - 1)
> + * and since m divides BLIST_BMAP_RADIX, we can simplify further to
> + * skip = (radix / (BLIST_BMAP_RADIX / m)) / (m - 1)
> + * skip = radix / ((BLIST_BMAP_RADIX / m) * (m - 1))
> + * so that simple integer division by a constant can safely be used for
> the
> + * calculation.
>   */
>  static inline daddr_t
>  radix_to_skip(daddr_t radix)
>  {
>
> return (radix /
> -   (BLIST_BMAP_RADIX / BLIST_META_RADIX * (BLIST_META_RADIX -
> 1)));
> +   ((BLIST_BMAP_RADIX / BLIST_META_RADIX) * (BLIST_META_RADIX -
> 1)));
>  }
>
>  /*
> ___
> svn-src-h...@freebsd.org  mailing list
> https://lists.freebsd.org/mailman/listinfo/svn-src-head
> To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org
> "
>
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

svn commit: r322041 - head/sys/kern

[Alan Cox]

Author: alc
Date: Fri Aug  4 04:23:23 2017
New Revision: 322041
URL: https://svnweb.freebsd.org/changeset/base/322041

Log:
  In case readers are misled by expressions that combine multiplication and
  division, add parentheses to make the precedence explicit.
  
  Submitted by: Doug Moore 
  Requested by: imp
  Reviewed by:  imp
  MFC after:1 week
  X-MFC after:  r321840
  Differential Revision:https://reviews.freebsd.org/D11815

Modified:
  head/sys/kern/subr_blist.c

Modified: head/sys/kern/subr_blist.c
==
--- head/sys/kern/subr_blist.c  Fri Aug  4 04:20:11 2017(r322040)
+++ head/sys/kern/subr_blist.c  Fri Aug  4 04:23:23 2017(r322041)
@@ -110,6 +110,7 @@ __FBSDID("$FreeBSD$");
 #definebitcount64(x)   __bitcount64((uint64_t)(x))
 #define malloc(a,b,c)  calloc(a, 1)
 #define free(a,b)  free(a)
+#define CTASSERT(expr)
 
 #include 
 
@@ -142,6 +143,8 @@ static void blst_radix_print(blmeta_t *scan, daddr_t b
 static MALLOC_DEFINE(M_SWAP, "SWAP", "Swap space");
 #endif
 
+CTASSERT(BLIST_BMAP_RADIX % BLIST_META_RADIX == 0);
+
 /*
  * For a subtree that can represent the state of up to 'radix' blocks, the
  * number of leaf nodes of the subtree is L=radix/BLIST_BMAP_RADIX.  If 'm'
@@ -151,17 +154,19 @@ static MALLOC_DEFINE(M_SWAP, "SWAP", "Swap space");
  * in the 'meta' functions that process subtrees.  Since integer division
  * discards remainders, we can express this computation as
  * skip = (m * m**h) / (m - 1)
- * skip = (m * radix / BLIST_BMAP_RADIX) / (m - 1)
- * and if m divides BLIST_BMAP_RADIX, we can simplify further to
- * skip = radix / (BLIST_BMAP_RADIX / m * (m - 1))
- * so that a simple integer division is enough for the calculation.
+ * skip = (m * (radix / BLIST_BMAP_RADIX)) / (m - 1)
+ * and since m divides BLIST_BMAP_RADIX, we can simplify further to
+ * skip = (radix / (BLIST_BMAP_RADIX / m)) / (m - 1)
+ * skip = radix / ((BLIST_BMAP_RADIX / m) * (m - 1))
+ * so that simple integer division by a constant can safely be used for the
+ * calculation.
  */
 static inline daddr_t
 radix_to_skip(daddr_t radix)
 {
 
return (radix /
-   (BLIST_BMAP_RADIX / BLIST_META_RADIX * (BLIST_META_RADIX - 1)));
+   ((BLIST_BMAP_RADIX / BLIST_META_RADIX) * (BLIST_META_RADIX - 1)));
 }
 
 /*
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"