On Wed, Jul 29, 2015 at 8:50 AM, Michael Matz <matz....@frakked.de> 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 _______________________________________________ Tinycc-devel mailing list Tinycc-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/tinycc-devel