Re: [Tinycc-devel] Source vandalism
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. See also the mob regression ... thread from 1.7.2015 and thanks in advance for reverting 4e04f67c94c9 on top of your changes :P -- gr ___ Tinycc-devel mailing list Tinycc-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Re: [Tinycc-devel] Source vandalism
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
Re: [Tinycc-devel] RE : [RFC] Moving source code to src
Hi, On Mon, 27 Jul 2015, gus knight wrote: On Mon, Jul 27, 2015 at 12:48 PM, Christian JULLIEN eli...@orange.fr wrote: Hi Gus, This is a good idea. If you do so, I also suggest an arch directory that contains subdirectories for all supported archives I wound up going with one dir per arch rather than an arch directory. Let me know if there are any regressions. $ cd tinycc $ ./configure make ... make: *** No targets specified and no makefile found. Stop. Sigh. Also make -C src is broken, the documentation in particular: $ make -C src ... ./texi2pod.pl ../docs/tcc-doc.texi tcc.pod make: ./texi2pod.pl: Command not found make: [tcc.1] Error 127 (ignored) pod2man --section=1 --center=Tiny C Compiler --release=`cat ./../VERSION` tcc.pod tcc.1 Can't open tcc.pod: No such file or directory at /usr/bin/pod2man line 60. make: [tcc.1] Error 2 (ignored) This is because you've moved texi2pod.pl from ./ into ./docs/ but haven't changed the makefile. Please fix. Ciao, Michael. ___ Tinycc-devel mailing list Tinycc-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Re: [Tinycc-devel] RE : [RFC] Moving source code to src
On 2015-07-29 09:00:18 -0400, gus knight wrote: Ah, oops. Looks like make didn't fail on the docs command as I expected it to. Also, don't forget to test out-of-tree builds with a read-only source directory. -- Vincent Lefèvre vinc...@vinc17.net - Web: https://www.vinc17.net/ 100% accessible validated (X)HTML - Blog: https://www.vinc17.net/blog/ Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon) ___ Tinycc-devel mailing list Tinycc-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/tinycc-devel
[Tinycc-devel] Source vandalism
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:
@@ -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:
-#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.
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.
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.
Sigh.
Ciao,
Michael.
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.
___
Tinycc-devel mailing list
Tinycc-devel@nongnu.org
https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Re: [Tinycc-devel] Source vandalism
On 2015-07-29 08:56:47 -0400, gus knight wrote: On Wed, Jul 29, 2015 at 8:50 AM, Michael Matz matz@frakked.de wrote: 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. Anyway, tabs are incorrect: with tabs, various tools break indentation, such as diff (thus patches may be difficult to read) and git blame. -- Vincent Lefèvre vinc...@vinc17.net - Web: https://www.vinc17.net/ 100% accessible validated (X)HTML - Blog: https://www.vinc17.net/blog/ Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon) ___ Tinycc-devel mailing list Tinycc-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Re: [Tinycc-devel] Source vandalism
On Wed, Jul 29, 2015 at 10:08 AM, Daniel Glöckner daniel...@gmx.net wrote: If you use clang-format again, please don't change char *x to char* x. No, I don't intend to. And I wasn't sure which was the correct thing to use, so I looked at some files and guessed. Sorry. -gus ___ Tinycc-devel mailing list Tinycc-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Re: [Tinycc-devel] Source vandalism
On Wed, Jul 29, 2015 at 08:56:47AM -0400, gus knight wrote: 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. The CodingStyle document is from April. Most code predates that file and I can't remember any discussion about the coding style on this list. The document merely lists what has been found while looking at several files. 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...) If you use clang-format again, please don't change char *x to char* x. Best regards, Daniel ___ Tinycc-devel mailing list Tinycc-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/tinycc-devel
Re: [Tinycc-devel] RE : [RFC] Moving source code to src
On Wed, Jul 29, 2015 at 8:58 AM, Michael Matz matz@frakked.de wrote: make: *** No targets specified and no makefile found. Stop. Sigh. Also make -C src is broken, the documentation in particular: $ make -C src ... ./texi2pod.pl ../docs/tcc-doc.texi tcc.pod make: ./texi2pod.pl: Command not found make: [tcc.1] Error 127 (ignored) pod2man --section=1 --center=Tiny C Compiler --release=`cat ./../VERSION` tcc.pod tcc.1 Can't open tcc.pod: No such file or directory at /usr/bin/pod2man line 60. make: [tcc.1] Error 2 (ignored) This is because you've moved texi2pod.pl from ./ into ./docs/ but haven't changed the makefile. Please fix. Ah, oops. Looks like make didn't fail on the docs command as I expected it to. -gus ___ Tinycc-devel mailing list Tinycc-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/tinycc-devel
