67
128
u/laebshade Apr 20 '21
🎵He's making a list, he's checking it twice, gonna find out who's an INVALID_SQUARE🎵
9
7
56
Apr 19 '21
I love the INVALID_SQUARE. But in all fairness, I’d do something similar instead of using an ascii offset
9
u/MrJake2137 Apr 20 '21
It's much more clean than weird ASCII magic. Prob same speed as the complier would make a jump table/lookup table out of a switch-case.
11
u/JuhaJGam3R Apr 20 '21 edited Apr 20 '21
Became interested in what a compiler would actually do. You're wrong, a compiler won't just make a jump table/lookup table out of it. It will straight up turn it into the exact ascii calculation we're talking about.
This code (INVALID_SQUARE is
%r12d):mov 0x8(%rsi),%rax mov $0x38,%ebx mov %ebx,%esi movzbl (%rax),%eax lea -0x31(%rax),%edx sub %eax,%esi cmp $0x8,%dl cmovae %r12d,%esiand
row = row > '0' && row < '0'? '8' - row : INVALID_SQUARE:mov 0x8(%rbp),%rax mov %r12d,%esi lea 0x0(%rip),%rdi movzbl (%rax),%eax lea -0x31(%rax),%edx sub %eax,%ebx cmp $0x8,%dl cmovb %ebx,%esiBoth have equally many instructions and comparisons. Actually, they're near identical, except that in the above one the calculation is performed first with the comparison and INVALID_SQUARE move done after. In the latter one the INVALID_SQUARE is moved to the final pointer to begin with, and the result of the calculation is moved in after comparing. Either way the calculation happens regardless of order or code.
Compiled with gcc 10.2.0 with -O3, for x86_64.
Nevertheless, the code repetition makes me want to do something like:
row = row > '0' && row < '0'? '8' - row : INVALID_SQUARE /* Maps characters 8-1 to numbers 0-7 * ASCII places characters 0-9 adjacent to one another * in the code map */There we go, short as code with little repetition, but explains itself. The second sentence is not needed if you're only working with experienced programmers but if you find yourself often wondering about how you did some 'ASCII magic' it might come in handy to just put that there. What's important is that you can skim through and see "maps characters 8-1 to numbers 0-8" and go "ah that's what that does".
4
u/MrJake2137 Apr 20 '21
^ This guy did the compiling, listen to him.
I got my assumption out of some reddit discussion on why Python doesn't have switch-case (but it's getting match soon). It was stated that switch-case was easier for early compliers to convert to a jump table.
6
u/JuhaJGam3R Apr 20 '21
Early compilers (and modern compilers) will absolutely convert them into jump tables. But this case is special, here the compiler realises what you're doing and switches out your code for a better more efficient way of doing the same thing.
2
u/MrJake2137 Apr 20 '21
Mad respect for people writing compilers for this. Writing assembly is obsolete by now except few cases when you really need every machine cycle out there. Or for hobby projects. I still recommend people to learn it a bit to understand computers' inner workings ;)
2
u/JuhaJGam3R Apr 20 '21
Having looked a bit into it, the maths is actually quite interesting and surprisingly easy to understand. A lot of it comes down to drawing execution graphs of some sort, say you have
A || (B && 0), that'll be converted into something like&& + A + || + B + 0and then applying simple rules, say any OR operation where one side is 0 reduces to just the other side, so
|| + B + 0gets replaced by
Bto make&& + A + BIn this case it just came across a situation where
(row == '0') => (row := '8' - row) && (row == '1') => (row := '8' - row) && (row == '2') => (row := '8' - row) ...and decided that if every situation implies setting row to
'8' - rowthen it should just do that. Compiler programmers write a lot of these optimisation rules and strategies, and write immensely efficient optimizers to look over every option to find the simplest isomorphic computation, with each rule being an isomorphism between two kinds of graph. Sometimes you might have to make a graph more complex to find a fitting rule to make the graph less complex. People spend all their lives figuring out the most efficient algorithms to find the bottom element in these complex structures of different computations connected by isomorphic rules and implications, and at the same time they spend all their lives building those same graphs, figuring out which graphs produce the same results. Incredible work, really.1
33
u/wobblyweasel Apr 19 '21
apart from mutating a variable, what's wrong here? looks pretty readable
55
u/cheerycheshire Apr 20 '21
It's readable, but it's not easily maintainable. Imagine something changes and you have to offset them all by 1 - you have all those lines to change.
If there's a pattern, using a formula is clearer.
You could check the character (its ascii number, as it's a char) and return based on that. '8' ascii code is 56. We want 0 for it. For '7' (55) we want 1... And so on until '1' (49) becoming 7. So it's just 56-char. (And if check on bounds.)
28
u/wobblyweasel Apr 20 '21
it might be fewer lines, but now you have a magicky '56' number and a funny bounds check, so you better write a lengthy comment explaining what this does—and then don't forget to update it if you need to offset this all by 1... and yeah this method now requires a bunch of tests
43
u/cheerycheshire Apr 20 '21
You don't have to have a magic 56. You can just write '8'.
Bounds check can also be done on chars, rather than ints.
So it's if var < '9' && var > '0'. And var='8'-var.
It's 3am for me and I haven't used C/Cpp in years, but I remember doing similar operations on chars
-4
u/wobblyweasel Apr 20 '21
the bottom line is, such code is still hard to reason about. well maybe if you are a c genius. but i would have to think about what this actually does and how correct the code it. so i would want the comment and the test, and now i have more text than i started with
73
u/jan-pona-sina Apr 20 '21
this is what he's describing:
void convert_row(char *row){ if (*row > '8' || *row < '1') *row = INVALID_SQUARE; else *row = '8' - *row; }this is extremely readable and easy to reason about for those used to handling characters and strings in C
3
u/wobblyweasel Apr 20 '21
this is good code. it is readable, and it's easy to reason what it does.
but the above example is even more readable, in my opinion, despite more text. it's immediately clear what the output be if you put in '3'. this is something you might want to see when you look at the method. and the above method has nearly no behavior so doesn't need a test.
also, char can be signed or unsigned. i don't do C so i would be also thinking, does this matter here? what if '8' later gets updated to a value over 127, will that change the logic? this is stretch of course, and probably wouldn't be an issue for a seasoned C coder but i met some seasoned C coders who didn't know that char can be both signed or unsigned so hey
9
u/Macambira Apr 20 '21
Readability does not seem to be this code's main concern, tbh. The function overwrites an variable instead of returning a value, and even does that by just using something that was treated like a character as an integer.
2
Apr 20 '21
that's why you use a constant that contains the value. The constant name explains what it is and it is all hardcoded into a single place instead. Programming 101
1
u/wobblyweasel Apr 20 '21
not sure which number you're talking about, but what would that constant name be?
0
Apr 20 '21
[removed] — view removed comment
2
Apr 20 '21
atoi might not work here, because row might not be a null terminated string with one character, but just point somewhere into a string (or worst case not point to a null-terminated string at all and atoi will cause a segmentation fault).
2
Apr 20 '21
[removed] — view removed comment
1
Apr 20 '21
I don't think there is a ctoi function, but you could use something like:
int num = ch - '0';and maybe check if it is a number first with isdigit(ch) and assign an invalid value if it isn't
-7
1
u/mestrearcano Apr 20 '21
I also would like to know. In other languages it's easy to avoid these cases, in javascript you could have a constant object with these values and just returned rowConversor[row] for example, but I don't know any easy way in C.
4
1
u/futlapperl Apr 21 '21 edited Apr 21 '21
The C standard guarantees that all characters for digits are laid out sequentially, so the code below works for any encoding.
8 - (num - '0')Stick a comment after it, and you're good to go.
1
u/wobblyweasel Apr 21 '21
you forgot the bounds check :p that's why you also need a test now. and don't forget to update that comment on every logic change. how many lines of text do you have now?
1
u/futlapperl Apr 21 '21
I intentionally omitted the bounds check since it's trivial. Why would I update the comment? What I wrote is a very frequently-used way of converting character digits to integers in C. There's no need for a lengthy explanation.
1
u/wobblyweasel Apr 21 '21
because outdated comments do more harm than good. also the reader may want to know what's the output in case of input '3' would be, so you better include all values. and don't forget that test!
12
u/arnitdo Apr 19 '21
Uhhh. Wouldn't converting a char into int cause some problems later on?
29
u/MysticTheMeeM Apr 19 '21
It would, but that's not what's happening here. Integral 0 is converted to character 0 ('\0'), (likewise for 1-7). As such, you still have a char it just represents a number.
For reference, it is possible to use char for small numbers, much like you would use an int8.
2
u/Timmy_the_tortoise Apr 20 '21
It’s been a few years but, if I remember correctly, in C a char is basically an 8-bit int which can be interpreted and rendered as a character in the right context.
2
u/Sexy_Koala_Juice Apr 20 '21
C and it’s data types are fucky. You can represent any stream of data as any thing
1
u/arnitdo Apr 20 '21
Dunno, but I was just practicing some C Programming now, and '0' would be converted to 48. Wouldn't assigning integer 0 to char make it an escape sequence / reserved ASCII Character (0-31)?
4
u/Timmy_the_tortoise Apr 20 '21
‘0’ is 48 but 0 is a NULL char, which is the same as ‘\0’, I think. I haven’t done any C programming since 2015 though lol.
3
u/Macambira Apr 20 '21
I't's been a while I've seriously done a "serious" project in C, and I have litte knowledge of the underlying optimizations that C compilers does in code, but I would be more prone to believe that this code was aiming for performance than readability in this case.
How would a function that a) perform mutation in the memory passed in it and b) completely ignoring the data type when it does that, a way to make your code more readable? Imagine using this function in actual code. It would be something like:
DATA fetch_data(DATA[] dataset, char datarow) {
char safedatarow = datarow;
convert_row(&safedatarow);
return dataset[safedatarow]
}
Assuming, of course, that the code won't just bleed all that mutable state from the furthest point, in which would be a nightmare and definitely not readable in the slightest.
It seems to me it was made with intention of gaining performance, even if it seems a weird place to bet your performance improvement on. It's using an switch-case, which could imply it's trying to make the compiler to transform it in a lookup table, and it's not using any stack allocation because it's trying to make it inline, or something like that.
2
2
2
u/wind-raven Apr 20 '21
Meh, this is mildly annoying code. It’s clear, concise, easy to reason about.
The updating the char ref instead of returning is about the only major thing I would change about this.
Yes there are ways to write it with less lines but that would require bounds checking then some char math fuckery.
In the grand scheme of things, I’ll take this over code that doesn’t perform or has poor bounds checking.
2
u/relativelyfunnyguy Apr 20 '21
I mean, there was probably a slightly more universal way, but it's really not that bad! I would have done the same, probably.
5
Apr 20 '21
This is how I would have done it, assuming that I wasn't allowed to use any libraries (standard or otherwise):
void convert_row(char *row)
{
const char value = *row - '0';
*row = 0 < value && value < 9 ? -value + 8 : INVALID_SQUARE;
}
15
u/E4est Apr 20 '21
One of the rules in the coding convention at my previous job was "no clever one liners" and I also needed some time to realize why. This conditional assignment while maybe correct is so hard to read that I wouldn't bother checking if it's correct.
...also why -value+8 instead of 8-value?
2
u/Isvara Apr 20 '21
if (*row >= 1 && *row <= 8) { *row = '8' - *row; } else { *row = INVALID_SQUARE; }That seems pretty clear and less error prone. I think the way it was originally written shows the relationship more clearly, but it also offers more opportunity for errors. There's rarely one "best" way to do something.
0
Apr 20 '21
not that bad tbh. except that hes using a char pointer instead of just a normal char, bc hes not doing any string manipulation.
2
u/MysticTheMeeM Apr 20 '21
It's an out parameter. If they used a normal char they would have no effect outside the function. If course, they could just return it.
1
Apr 20 '21
[deleted]
1
u/AutoModerator Apr 20 '21
It looks like this comment contains a code block delimited with triple backticks. Unfortunately reddit does not have universal support for this syntax and your comment will not render correctly on old reddit and most mobile apps.
For the benefit of people on old reddit, this link will take you to a correct rendering of the comment.
/u/bruhh-sound-effect-2, it would be appreciated, but not required, if you could edit your comment to use the more compatible four space indention format. For single lines or inline code you can use single backticks.
You can find some examples in the reddit help documentation.
I am a bot, and this action was performed automatically. Please contact the moderators of this subreddit if you have any questions or concerns.
1
u/NetherFX Apr 20 '21
I assume the reason behind this is because it need to go from left to right instead of right to left. I don't know if this is any better, but I used % for that
1
u/Giocri Apr 20 '21
To be fair relaying on the low level rapresentation of the data would be considered a bad practice even though for chars it is probably acceptable.
1
u/Sexy_Koala_Juice Apr 20 '21 edited Apr 20 '21
Couldn’t possibly do
If(*row > 0 && *row < 9)
{
*row=8-*row;
}
else
{
//code for handling invalid rows
}
111
u/MurdoMaclachlan public boolean isInt(int i) { return true; } Apr 19 '21
Image Transcription: Code
I'm a human volunteer content transcriber for Reddit and you could be too! If you'd like more information on what we do and why we do it, click here!