Re: [Tinycc-devel] Source vandalism

[gus knight]

On Wed, Jul 29, 2015 at 8:50 AM, Michael Matz <address@hidden> wrote:
> Hello gus or Augustin,
>
> while I appreciate more people working on tinycc, why do you think the
> best thing to do as the very first commits would be source code
> reformattings and reorganizations?  Look at what damage you've done:

The reformattings were to attempt to make everything conform to the
"CodingGuidelines" document that was already in the tree (4 spaces not
tabs, etc.) which a few files didn't conform to.

> @@ -204,7 +204,7 @@ void C67_g(int c)
>  #endif
>      ind1 = ind + 4;
>      if (ind1 > (int) cur_text_section->data_allocated)
> -       section_realloc(cur_text_section, ind1);
> +    section_realloc(cur_text_section, ind1);
>
> Grand, so now the conditioned statement isn't indended anymore.  Or:
>
>      while (t) {
> -       ptr = (int *) (cur_text_section->data + t);
> -       {
> -           Sym *sym;
> +    ptr = (int *) (cur_text_section->data + t);
> +    {
> +        Sym *sym;
>
> The 'ptr =' assignment has to be indended.  Or:
>
>
>                  } else {
> -                   qrel->r_info = ELFW(R_INFO)(0, R_X86_64_RELATIVE);
> -                   qrel->r_addend = *(long long *)ptr + val;
> +            qrel->r_info = ELFW(R_INFO)(0, R_X86_64_RELATIVE);
> +            qrel->r_addend = *(long long *)ptr + val;
>                      qrel++;
>                  }
>
> What the hell?  Parts of the conditional block are now indended completely
> wrong.  Or such changes:

Yes, I'm sorry, this is a mistake. I was trying to convert the tabs to
spaces, but it appears that there was a variety of styles (some people
used 8 spaces per tab, some used 4...) so my automated conversion
methods didn't work so well.

> -#define RC_INT     0x0001 /* generic integer register */
> -#define RC_FLOAT   0x0002 /* generic float register */
> -#define RC_R0      0x0004
> -#define RC_R1      0x0008
> -#define RC_R2      0x0010
> -#define RC_R3      0x0020
> -#define RC_R12     0x0040
> -#define RC_F0      0x0080
> -#define RC_F1      0x0100
> -#define RC_F2      0x0200
> -#define RC_F3      0x0400
> +#define RC_INT 0x0001   /* generic integer register */
> +#define RC_FLOAT 0x0002 /* generic float register */
> +#define RC_R0 0x0004
> +#define RC_R1 0x0008
> +#define RC_R2 0x0010
> +#define RC_R3 0x0020
> +#define RC_R12 0x0040
> +#define RC_F0 0x0080
> +#define RC_F1 0x0100
> +#define RC_F2 0x0200
> +#define RC_F3 0x0400
>
> Well, obviously the author of this wanted to align the numbers like in a
> table, now it looks ugly.  Or this:
>
>  static unsigned long put_got_entry(TCCState *s1,
> -                                  int reloc_type, unsigned long size, int 
> info,
> -                                  int sym_index)
> +                   int reloc_type, unsigned long size, int info,
> +                   int sym_index)
>  {
>
> The arguments on second and third line were meant to align with the first
> argument.  Or just a few lines down:
>
>      if (need_plt_entry && !s1->plt) {
> -       /* add PLT */
> -       s1->plt = new_section(s1, ".plt", SHT_PROGBITS,
> -                             SHF_ALLOC | SHF_EXECINSTR);
> -       s1->plt->sh_entsize = 4;
> +    /* add PLT */
> +    s1->plt = new_section(s1, ".plt", SHT_PROGBITS,
> +                  SHF_ALLOC | SHF_EXECINSTR);
> +    s1->plt->sh_entsize = 4;
>      }
>
> Gnah!  First the whole conditional statements aren't indended anymore, and
> second the arguments to the new_section call aren't aligned.
>
> This is all quite obvious and terrible, and I could go on and on just
> citing from the diffs.  Have you even _looked_ at your patches before
> committing them?  Fix all of this right away, otherwise we have to revert
> the whole series.

Yes, I will. Apologies, I should've looked more closely (or run the
whole thing through clang-format, but that probably would've done
damage in a different way...)

> Also there's another argument against generally doing such code
> reformattings: git blame is disturbed now, all lines that you've
> reindended now show your commit as the one to blame, which is useless
> information.  Unfortunately that's a damage that can't be undone now
> anymore.

I seem to recall there's some way to have "git blame" ignore
whitespace-only commits? I could be mistaken...

> Next time you want to do whole-sale code changes please discuss on the
> mailing list before doing so, there might be reasons for the status quo
> that you're unaware of, like in this case.

Noted.

> P.S: some of the reindendation changes look like as if you've replaced
> leading tabs with four spaces.  That would have been wrong, tabs are eight
> spaces.  If this is the case, fix your editor settings.

In some of the cases, it's obvious that whoever wrote the code was
using 4 spaces per tab, but in others it's obvious they were using 8.
It was kind of hard to determine which, and I had to second-guess
about the surrounding code.

-gus