Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

54-line if condition in gcc's reload.c

github.com

Post reply on HN
Print view View original

31–40 of 95 posts

Re: 54-line if condition in gcc's reload.c

#31

Post by hrktb »

mkl wrote: post #22
goblin89 wrote: post #15

Why put logical operator at the start and not the end of each line? I.e., this style (used in this case) && (CONSTANT_P (SUBREG_REG (in)) || GET_CODE (SUBREG_REG (in)) == PLUS || strict_low || (((REG_P (SUBREG_REG (in)) versus this style: (CONSTANT_P (SUBREG_REG (in)) || GET_CODE (SUBREG_REG (in)) == PLUS || strict_low || (((REG_P (SUBREG_REG (in)) && I don't have a personal preference here, just looking for any prac…

I used to use the latter, in both code and maths, but I have switched to the former. My main reason is that having the operators (logical and otherwise) at the start of the line means they're more likely to be in the same horizontal position, so it's easier to see which lines are continuations of the previous ones.

This, plus the stucture stays very readable even if for any specific reason a test clause has a very long line.

Re: 54-line if condition in gcc's reload.c

#32

Post by matthewmacleod »

raldi wrote: post #10
dokem wrote: post #4

Why not break it up into variables or functions so that the logic can be followed while reducing the likelihood that a bug creeps in? What is the excuse for horrible code like this?

It's been getting the job done for decades, in perhaps the most popular compiler of all time. I'm willing to trust the maintainers' judgement on this one. If they'd spent all their time needlessly fixing what ain't broke, gcc would've gone the way of GNU/Hurd.

You could say exactly the same about OpenSSL - it's not a good argument.

Re: 54-line if condition in gcc's reload.c

#33

Post by VMG »

webkike wrote: post #8
adamnemecek wrote: post #6

No wonder llvm is getting popular (I realize that there are other reasons besides codebase quality).

Compilers are complex, and these conditionals had to be evaluated in some way. Sure the author could have split it up into multiple if statements, but would have that really helped?

You could pull out boolean subexpressions and give them descriptive names and still use one if statement. It's even in the Google C++ style guide I believe.

Re: 54-line if condition in gcc's reload.c

#34

Post by laichzeit0 »

adamnemecek wrote: post #9
webkike wrote: post #8

Earlier quoted context omitted.

Compilers are complex, and these conditionals had to be evaluated in some way. Sure the author could have split it up into multiple if statements, but would have that really helped?

Yes, yes it would have, there is literally no question about that. It also appears that some of the parts of the conditions are repeated, so those could have been refactored.

I'm sure a good optimizing compiler like GCC would optimize out repeated expressions in conditions ;) (perhaps not if they're declared volatile(?))

Re: 54-line if condition in gcc's reload.c

#35

Post by DannyBee »

bdcravens wrote: post #17
AYBABTME wrote: post #2

Trivia: This code is older than many of the readers. https://github.com/mirrors/gcc/blame/7057506456ba18f080679b2...

I'm impressed that the blame log has survived intact. What SCM was originally used?

So, i converted the repository to SVN (which is what this is based on).

Originally? None.

Then RCS

Then it forked into EGCS and GCC, and EGCS used CVS

On remerge, we used CVS.

Then we converted to SVN.

I basically rewrote large parts of cvs2svn to make this happen (before that it took weeks to convert and ran out of memory anyway :P)

During cvs2svn conversion, the old GCC RCS versions we had data for were inserted as branches and older revisions, as appropriate

Since the original per-file version numbers were not kept, this was done by inserting it and then incrementing existing version numbers on the RCS files that made up the CVS repository, and hacking up the cvs2svn parser slightly to allow for some idiosyncracies (because now you have a billion years worth of RCS bugs to parse)

So basically, history goes back as far as there were version control systems that were being used, which is roughly 1987

Before someone asks why it got moved to SVN, at the point at which we converted to SVN,

1. git was not popular yet (or all that usable yet for that matter)

2. GCC's biggest VC problem was needing single repository version numbers for tags, and atomic commits, which SVN solved.

SVN was a huge step forward compared to CVS.

Re: 54-line if condition in gcc's reload.c

#36

Post by Strilanc »

goblin89 wrote: post #15

Why put logical operator at the start and not the end of each line? I.e., this style (used in this case) && (CONSTANT_P (SUBREG_REG (in)) || GET_CODE (SUBREG_REG (in)) == PLUS || strict_low || (((REG_P (SUBREG_REG (in)) versus this style: (CONSTANT_P (SUBREG_REG (in)) || GET_CODE (SUBREG_REG (in)) == PLUS || strict_low || (((REG_P (SUBREG_REG (in)) && I don't have a personal preference here, just looking for any prac…

I prefer the operators at the start because the start is less jagged, so the operators don't get spread out so much and stand out more.

Usually I only put them at the end of a line in languages that do implicit line endings, and will false-positive when you move the operator to the next line (i.e. javascript and visual basic).

Re: 54-line if condition in gcc's reload.c

#38

Post by MaulingMonkey »

raldi wrote: post #10
dokem wrote: post #4

Why not break it up into variables or functions so that the logic can be followed while reducing the likelihood that a bug creeps in? What is the excuse for horrible code like this?

It's been getting the job done for decades, in perhaps the most popular compiler of all time. I'm willing to trust the maintainers' judgement on this one. If they'd spent all their time needlessly fixing what ain't broke, gcc would've gone the way of GNU/Hurd.

> It's been getting the job done for decades, in perhaps the most popular compiler of all time. I'm willing to trust the maintainers' judgement on this one.

I don't trust anyone - myself included - writing code 1/10th as convoluted, age-of-product be damned. Code is not wine, old doesn't mean good. Neither do the maintainers, methinks: Looking at blame shows some refactoring.

> If they'd spent all their time needlessly refactoring, gcc would've gone the way of GNU/Hurd.

And the lack of needful refactoring may very well send it the way of COBOL - with everyone merely wishing it had gone the way of GNU/Hurd. I've seen clang and LLVM replace gcc in both of my vendor toolchains that used gcc - suggesting they've already wished and then done something about that wish.

(Either that or licensing, but code like this stomps on the scales a bit...)

Re: 54-line if condition in gcc's reload.c

#39

Post by tempestn »

dokem wrote: post #4

Why not break it up into variables or functions so that the logic can be followed while reducing the likelihood that a bug creeps in? What is the excuse for horrible code like this?

Because the choice is never, "Should I refactor this code or not?" The choice is, "Should I refactor this code, or work on this feature/bug/refactoring/etc. instead?" Sure it could be refactored to be cleaner, but thus far, presumably there has always been something more important to do.

Any time this if statement has been edited, it was clearly faster to simply add to the existing one than to refactor the whole thing. Sometimes it makes sense to take that extra time to refactor code when you happen to be working on it anyway... and sometimes there's something else more pressing to do.

Re: 54-line if condition in gcc's reload.c

#40

Post by [deleted] »

mkl wrote: post #22
goblin89 wrote: post #15

Why put logical operator at the start and not the end of each line? I.e., this style (used in this case) && (CONSTANT_P (SUBREG_REG (in)) || GET_CODE (SUBREG_REG (in)) == PLUS || strict_low || (((REG_P (SUBREG_REG (in)) versus this style: (CONSTANT_P (SUBREG_REG (in)) || GET_CODE (SUBREG_REG (in)) == PLUS || strict_low || (((REG_P (SUBREG_REG (in)) && I don't have a personal preference here, just looking for any prac…

I used to use the latter, in both code and maths, but I have switched to the former. My main reason is that having the operators (logical and otherwise) at the start of the line means they're more likely to be in the same horizontal position, so it's easier to see which lines are continuations of the previous ones.

[deleted]
Post reply on HN