[MPlayer-cvslog] r27081 - trunk/libmpdemux/demuxer.c

[Michael Niedermayer]

On Mon, Jun 16, 2008 at 08:33:35PM +0200, Benjamin Zores wrote:
> Ivan Kalvachev a écrit :
[...]
> > Not to say that so far Diego have done 12 fully cosmetics commits over single .c file. There are above 1900 .[ch] files, would we need 22800 commits to clean them up?.
> > 
> > I highly doubt this would be over in 24 hours.
> 
> Wasting human time on what can be done by machine is imho a bad idea.
> 
> The only solution to me for that is:
> - developers agree on a coding style (will take 6 months)
> - it has to be reformated through "indent 
> _the_necessary_options_for_this_style"
> - optionally add a "make indent" rule that call it on all .[hc] files
> - best would be afterwards to have an svn handler that automatically 
> reindent patches if badly indented.

The problem is that indent is not very smart ...

a simple test on a small file from ffmpeg (aes.c) with uotis "indent -kr"

static void subshift(uint8_t s0[2][16], int s, uint8_t *box){
    uint8_t (*s1)[16]= s0[0] - s;
    uint8_t (*s3)[16]= s0[0] + s;
    s0[0][0]=box[s0[1][ 0]]; s0[0][ 4]=box[s0[1][ 4]]; s0[0][ 8]=box[s0[1][ 8]]; s0[0][12]=box[s0[1][12]];
    s1[0][3]=box[s1[1][ 7]]; s1[0][ 7]=box[s1[1][11]]; s1[0][11]=box[s1[1][15]]; s1[0][15]=box[s1[1][ 3]];
    s0[0][2]=box[s0[1][10]]; s0[0][10]=box[s0[1][ 2]]; s0[0][ 6]=box[s0[1][14]]; s0[0][14]=box[s0[1][ 6]];
    s3[0][1]=box[s3[1][13]]; s3[0][13]=box[s3[1][ 9]]; s3[0][ 9]=box[s3[1][ 5]]; s3[0][ 5]=box[s3[1][ 1]];

gets changed to:

static void subshift(uint8_t s0[2][16], int s, uint8_t * box)
{
    uint8_t(*s1)[16] = s0[0] - s;
    uint8_t(*s3)[16] = s0[0] + s;
    s0[0][0] = box[s0[1][0]];
    s0[0][4] = box[s0[1][4]];
    s0[0][8] = box[s0[1][8]];
    s0[0][12] = box[s0[1][12]];
    s1[0][3] = box[s1[1][7]];
    s1[0][7] = box[s1[1][11]];
    s1[0][11] = box[s1[1][15]];
    s1[0][15] = box[s1[1][3]];
    s0[0][2] = box[s0[1][10]];
    s0[0][10] = box[s0[1][2]];
    s0[0][6] = box[s0[1][14]];
    s0[0][14] = box[s0[1][6]];
    s3[0][1] = box[s3[1][13]];
    s3[0][13] = box[s3[1][9]];
    s3[0][9] = box[s3[1][5]];
    s3[0][5] = box[s3[1][1]];

Now while the shorter lines can be liked or not, destroyed alignment makes
the code look quite ugly and makes it very hard to read.

 
-static inline int mix_core(uint32_t multbl[4][256], int a, int b, int c, int d){
+static inline int mix_core(uint32_t multbl[4][256], int a, int b, int c,
+                          int d)
+{

Id move a,b,c,d to the next line or even static inline to the previous or id
just leave it, the 1 char over 80 isnt that terrible.
Moving just int d looks random.


 #ifdef CONFIG_SMALL
 #define ROT(x,s) ((x<<s)|(x>>(32-s)))
-    return multbl[0][a] ^ ROT(multbl[0][b], 8) ^ ROT(multbl[0][c], 16) ^ ROT(multbl[0][d], 24);
+    return multbl[0][a] ^ ROT(multbl[0][b], 8) ^ ROT(multbl[0][c],
+                                                    16) ^
+       ROT(multbl[0][d], 24);

This as well is just not sanely split

    return       multbl[0][a]      ^ ROT(multbl[0][b],  8)
           ^ ROT(multbl[0][c], 16) ^ ROT(multbl[0][d], 24);

above which is manually split really looks nicer.


-static inline void mix(uint8_t state[2][4][4], uint32_t multbl[4][256], int s1, int s3){
-    ((uint32_t *)(state))[0] = mix_core(multbl, state[1][0][0], state[1][s1  ][1], state[1][2][2], state[1][s3  ][3]);
-    ((uint32_t *)(state))[1] = mix_core(multbl, state[1][1][0], state[1][s3-1][1], state[1][3][2], state[1][s1-1][3]);
-    ((uint32_t *)(state))[2] = mix_core(multbl, state[1][2][0], state[1][s3  ][1], state[1][0][2], state[1][s1  ][3]);
-    ((uint32_t *)(state))[3] = mix_core(multbl, state[1][3][0], state[1][s1-1][1], state[1][1][2], state[1][s3-1][3]);
+static inline void mix(uint8_t state[2][4][4], uint32_t multbl[4][256],
+                       int s1, int s3)
+{
+    ((uint32_t *) (state))[0] =
+        mix_core(multbl, state[1][0][0], state[1][s1][1], state[1][2][2],
+                 state[1][s3][3]);
+    ((uint32_t *) (state))[1] =
+        mix_core(multbl, state[1][1][0], state[1][s3 - 1][1],
+                 state[1][3][2], state[1][s1 - 1][3]);
+    ((uint32_t *) (state))[2] =
+        mix_core(multbl, state[1][2][0], state[1][s3][1], state[1][0][2],
+                 state[1][s1][3]);
+    ((uint32_t *) (state))[3] =
+        mix_core(multbl, state[1][3][0], state[1][s1 - 1][1],
+                 state[1][1][2], state[1][s3 - 1][3]);
 }

above is another nice example where indent really messes up.

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The misfortune of the wise is better than the prosperity of the fool.
-- Epicurus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-cvslog/attachments/20080616/d9358ed2/attachment.pgp>