We’re rewarding the question askers & reputations are being recalculated! Read more.

Hot answers tagged

11

Avoid global variables - there's no need for t and v to exist outside main(). Always check the return value of scanf() before using the written values. Don't assume that CHAR_BIT is 8, or that sizeof (short) is 2. Neither of those is portable. Don't assume a particular ordering of bit fields within a struct - that's entirely compiler-dependent. Portable ...


10

Minor stuff ... Allocate to the object, not type The below is easier to maintain. // char **aux = malloc(sizeof(char *)) char **aux = malloc(sizeof *aux) // aux[0] = malloc(bufsize * sizeof(char)); aux[0] = malloc(sizeof *(aux[0]) * bufsize); Avoid Exploit Below code is undefined behavior is the first character of user input is the null character. It ...


10

The most serious problem I see here is failing to check the return value from scanf(). Never ignore that - it's the only way we have to determine whether conversions were successful. A less serious issue is that we we're using a main() that takes command-line arguments, but never use them. Prefer instead the no-argument version: int main(void) The ...


9

Use descriptive variable names Instead of int a, b, c, give them more descriptive names: int day, month year; sscanf() will ignore trailing garbage The function sscanf() will stop parsing after the last conversion. So the string "1.2.3...." will be cleared by your check for digits and period characters, and then sscanf() will read 3 integers, and returns ...


7

So, you basically rewrote the strtok function in ANSI C. The difference is that you allocate memory for each substring while TOK modifies the original string by adding \0 characters in the place of delimiters. This means that you keep allocating more and more memory, while you can just make a copy of the whole string and use strtok to modify the copy you ...


5

All the code looks like you are very experienced since you didn't make any obvious mistakes. Some small things to consider: I'd compile the release binary with assertions enabled since I prefer an obvious crash over undefined behavior. Since you don't include <assert.h> at all, you don't need the -DNDEBUG flags at all since they won't make any ...


5

In addition to @Toby Speight's answer. There are only four types allowed for a bit field. There are the follows: signed int, unsigned int, int, and _Bool. So using char in this case: struct inside{ char p:8; char q:8; }; is implementation-defined. Use fixed integer types. In your code you use short the width of which is not well defined. If you ...


5

Complexity Breaking problems into smaller and smaller parts until it is easy to solve is a standard part of software design and programming. Small functions make it easier to write, read, debug and maintain code. Most variables will be local variables and that cleans up the code as well. The function main() is too complex (does too much). As programs grow ...


4

Do not cast the result of malloc. If your code does #include <stdlib.h>, the cast is redundant. If it does not, the cast just masks the warning, which may lead to hard to find bugs. Prefer sizeof(object) to sizeof(type). The latter leads to the double maintenance problem, in case the type is changed. I strongly recommend to have a constructor-like ...


4

Bug strlen("\x80\x80\x80\x80\x80\x80\x80") does not return the expected 7. The do loop does not distinguish between '\0' and '\x80'. strlen("\x80\x80\x80") returns 0, 1, 2 or 3 depending on alignment. These render the function broken. Why int32_t? Certainly int32_t* marches down the sting at a 4x clip rather than a plodding char *. Yet for a 16-bit ...


4

String as formats Code uses printf(howManySides); yet howManySides is not described as a format. Such practice can lead to trouble when the string contains a %. Do not use a string as a format unless it is clearly stated in its definition that it is a format. // printf(howManySides); fputs(howManySides, stdout); Ensure output before input stdout may be ...


4

Potential bugs I just spent a few minutes reviewing this, so I'm not sure these are bugs, but you should have a look at them: The comma operator is not the same as &&. You have this code: for (int i = 0; i < ipSrcLen, addrIdx < 4; i++) { but I think you meant to use && instead of ,. I'm not sure it matters because I'm not sure what ...


3

I agree with most of the suggestions from @TobySpeight, except for the loop variable. Consider: #include <stdlib.h> #include <stdio.h> int main(void) { short x; if (scanf("%hi", &x) != 1) { perror("Input error"); return EXIT_FAILURE; } unsigned short v = (unsigned short)x; for (int i = 8*sizeof(v)-1; i &...


3

We can eliminate the magic number 12 here: /* If we have a name for this polygon, then get it from the table. */ if(numSides <= 12) { name = names[numSides - 3]; } The common idiom for determining the number of elements in an array works by using the size of the array and the size of each element: static const int max_sides = sizeof names / sizeof *...


3

Welcome to the wonderful world of C programming where you have to do everything yourself. The C programming language is basically a high level assembly language, there is no garbage collection and the programmer has to manage the memory themselves. The Good Before I start discussing what can be improved, I'll mention the good things: - The code is quite ...


2

Firstly, the iterative approach is unnecessary, as we know the minimum must be at x = -b/a. I'll assume that the use of a quadratic function here is supposed to be illustrative of a more general function. There's an unnecessary include of <stdlib.h>, so we can drop that. return 0; is not needed at the end of main(), so we can omit that too. It's ...


2

1. Determination the length of an array Look at the marked line: int freeSpace() { for (int i = 0; i < sizeof(board); i++) // <---- { if (board[i] == ' ') { return true; } } } This code works because board has type char [9]. For this reason sizeof(board) equals to the number of elements in board1. But ...


2

Compiler Options and Warning Checking It might be a good idea to use the -wall compiler switch. It will provide warning messages that may indicate possible logic errors in some cases. When compiled with -wall this program yields the following warning messages: D:\ProjectsNfwsi\CodeReview\tictactoev1\main.c(89) : warning C4716: 'printBoard': must return a ...


2

(kind of nitpicking here, it looks quite solid to me) Code: /* Discard up until the last `0xFF`, then that is the marker type. */ do { if(fread(f, 1, 1, fp) != 1) return 0; } while(f[0] != 0xFF); do { if(fread(f, 1, 1, fp) != 1) return 0; } while(f[0] == 0xFF); While we might have extra 0xff fill to read, I'm not sure we should be reading and ...


2

Use X-Macro to construct parallel enum and array As the number of items increases, so does the effort required to keep the parallel constructions in sync. enum Combinator { POP, DUP, SWAP , FLIP, ID, QUOTE , UNQUOTE, UNKNOWN }; static const char Str_Combs[][8] = { "pop", "dup", "swap" , "flip", "id", "quote" , "unquote" }; Instead,...


2

As noted, the zero detection trick would also detect any 0x80 bytes (€ in CP1252, various different characters in UTF-8 contain 0x80 as continuation byte, for example Hiragana mu: む = "\xE3\x82\x80") as if they were zero-terminators. There are slightly more expensive "contains zero byte" checks that avoid this, for example (sprinkle with parenteses as ...


2

You cannot write this kind of code in standard C. It will have to be library-level code compiled with non-standard extensions. Notably, s = (uint32_t *)cptr; is a strict aliasing violation. It invokes undefined behavior and may cause strange, subtle bugs during optimization etc. In order to get this to work reliably, you have to use non-standard options ...


2

Small review Bug case 'u': { ... *ptr = strtol_s(token, base, &strtolErr); can form the wrong value as strtol() can limit values to 231-1. Same with 'x'. Better to use strtoul(). Type conflagration long strtol_s() looks like a function to convert a string to long yet it errors when out of int range and is curiously used in the code only for ...


2

Your compiler should be smart enough to replace the division by multiplication. Here's an experiment I did with CLang on x86_64: #include <inttypes.h> uint8_t div8_100(uint8_t a) { return a / 100; } uint8_t div8_10(uint8_t a) { return a / 10; } clang -O3 -Wall -Weverything -S div8.c div8_100: movzbl %cl, %eax leal (%rax,%rax,4), %ecx ...


1

You can use the standard C function sprintf(buffer,"%d",x) to get an array of digits. In this case, the buffer will contain big-endian representation of x, so the conversion to a digit from each char will be something like this for(int i = 0; buffer[i] != '\0' && i < sizeof(buffer);++i) digits[i] = buffer[i] - '0'; This work if x is positive. ...


1

C's string implementation is null-terminated. Thus, as long as, one doesn't care about the original string, (this may not be possible if you have a const char *,) and one has at least one char that one is willing to discard per block, (usually the block delimiter,) to turn into '\0', one can do this in place without copying. This tokenizing is very common, ...


1

With numeric tasks that could yield a wide range of answers, more informative to use "%e" than "%f" - else small non-zero values appear as "0.000000". // printf("min y is %f",min_y); printf("min y is %e\n",min_y);


1

There's not much to review here, since it's only a single macro, but there's still room for improvement. Don't use a macro I intend it to be a macro, so that it doesn't require any additional object file If that's the motivation for using a macro, it's based on a misconception. You can get that and avoid the problems of macros, by defining a static ...


Only top voted, non community-wiki answers of a minimum length are eligible