New answers tagged

2

#include <stdlib.h> This is not required - we use nothing declared by that header. void swap_numbers(void *a, void *b) { int temp = *(int *)a; Avoid void! We don't need to erase and reinstate type information like that: static void swap_unsigned(unsigned *a, unsigned *b) { unsigned temp = *a; *a = *b; *b = temp; } If we'll need this for ...


3

Unnecessary function calls My quick thought is that you are making an unnecessary function call. unsigned temp; while ((temp = a % b)) { a = b; b = temp; } The thing with the doubled parentheses is a hack to keep some compilers from warning you that you may be doing an unintentional assignment. It should make no functional difference. This does ...


1

Readability and (light) maintainability Readability in code is highly subjective, so take everything in this section with a grain of salt. Line lengths Most styles used in the wild will explicitly limit the length of the lines to less than 100 characters if possible. Whether it's 72, 80, 96 or another random value isn't important, but >140 is too much. ...


3

I would find it easier to understand if the comments were in English, but I'm not the one who will be making changes later, so write your comments for that person (whatever is most useful to future-you). What I find actively harmful is the very long lines such as //----------------------------------------------------------------------------------------------...


0

If you try this in MacOS, you’ll have an extreme fight on your hands. MacOS will at boot time install code optimised for your particular processor in a fixed place, this is done for memcpy, memmove , memset plus memset for two, four or eight byte values, and for some atomic operations. The memcpy on my current computer uses vector instructions, uses caching ...


5

#define handle_error(msg) do { perror(msg); exit(EXIT_FAILURE); } while (0) There's a long-standing convention to use UPPER_CASE for preprocessor macros. This helps draw attention to them as text replacements, which don't obey the same rules of scope as C identifiers (and which sometimes - though not in this case - expand their arguments multiple times). ...


1

Typically you'd want to use either \b or \r to let the new output overwrite the old output without clearing the whole screen. #include <stdio.h> #include <unistd.h> #define elements(x) (sizeof(x)/sizeof(x[0])) int main(){ char wheel[] = {'\\','|','/', '-'}; for (int i=0; i<30; i++) { printf("\r%c", wheel[i%...


6

Fix buffer overrun in copy_small As you've currently written it, and as Toby previously pointed out, copy_small always writes 8 bytes to dest, even when size < 8. This is a major memory safety bug as it writes past the end of the dest buffer. void copy_small(void *restrict dst, const void *restrict src, size_t size) { const uint64_t *restrict src64; ...


1

You aren't ever interested in where the nth part of your snake is, but whether one (or sometimes any other obstacle) is at a specific place. Lay out your data-structures accordingly! Have a single array representing the whole playing field, with codes reserved for space, edibles, walls, snake-head, and 4 types of snake-body (depending on where the body went)....


0

The wheel array is missing the horizontal spoke of the wheel -. The wheel array does not need a number to initialize it. char wheel[] = {'\\','|','/'}; The size of the array can be determined by: size_t wheel_size = sizeof(wheel) / sizeof(*wheel); Prefer size_t over int when indexing arrays. size_t is unsigned and can prevent a negative value being ...


0

The problem was that before going to sleep i had to flush the contents of stdout manually and that is why i did get nothing on the screen since printf buffers the output i either have to have a new line character which tells the buffer to flush or use fflush(stdout) before i call the sleep function. This answer was useful. https://stackoverflow.com/questions/...


0

If you're running windows the command is "CLS" not "clear". Learning modulo will help you a lot in programming - here you could use wheel[j%3] and skip the variable i. Also try to find a way to move the cursor instead of clearing the entire screen.


4

const uint64_t *restrict src64; Portability problem - uint64_t is only defined if the platform has a type of exactly 64 bits. Consider using uint_fast64_t (or perhaps uintmax_t) instead. pages = words / 8; offset = words - pages * 8; Another portability problem - assumes CHAR_BIT is 64/8 = 8. Use sizeof *src64 instead. The terminology is strange and ...


4

A couple of things. It’s a good idea to future-proof your program by declaring which version of the Windows SDK you’re using. This way, a future version of the compiler knows what version to stay compatible with and not to deprecate any functions you use. You can do this with the macros in <SdkDdkVer.h>, typically: #define _WIN32_WINNT ...


9

Enums In the code s is used to indicate the movement direction. From the code it's not easy to tell what s or s == 2 means. An enum would improve readability and maintainability of the code. typedef enum { Up, Down, Left, Right } Direction; Which would lead to something like this: Direction direction; if (kbhit()) { switch (getch()) { ...


2

I wanted to be able to have individual bit access in C without using a ton of functions to do bit manipulation on a uint8_t All things with bit-fields invite implementation defined behaviors. Even using unsigned char is implementation defined. A bit-field shall have a type that is a qualified or unqualified version of _Bool, signed int, unsigned int, or ...


23

You should keep your game loop as simple as possible with only an update and draw method and specifically in that order. To ensure your game loop is iterating at the same speed everytime, you should calculate the duration of your update and draw methods and then subtract that from how long each iteration should last. i.e.: MD = 10ms // max duration for an ...


2

We don't need to store the x and y coords in the Cell itself. We can calculate them cheaply when iterating through the grid. We don't need to store the previous game grid. When doing the update in TimeStep we can set lastAlive to 0 for live cells, and increment it for all dead cells (and perhaps clamp to a max value to prevent overflow). It would be better ...


4

This code is not portable. The order of bit-fields within a word is completely compiler-dependent, so the test that appears to work on one platform may give completely different results on another. You have avoided a common trap of using signed 1-bit fields (which can hold values 0 and -1) - these unsigned ones are much better. I don't think there's any ...


7

Answers to your questions I've read that functions shouldn't normally take more than 2–3 parameters I wouldn't worry about that too much. If you do have lots of parameters though, you should think about whether they are really necessary, and perhaps if there is a way to group them into a struct, and whether to pass that by value or by pointer. For example, ...


1

Implementation simplicity As code is incorrect in too many cases, simplicity assessment is moot. Ability to be used on different sorts of streams (writing into a file, a string, etc) Code assumes it can pass a void * like a FILE *. Reasonable, but not specified by C. Writing to a string deserves a size parameter. Rather than attempt to use fprintf(), ...


2

A key missing specification is the precision needed for the result as that steers algorithm design. Going forward that arg may be wide ranging and table look-up precision is good enough: I'd recommend simplifications: Pass the table in via global unless there might be more than 1 table. Perform as much as possible with integer math. With such low ...


4

Lots of good info in the other answers, so here are a few minor points. Use <stdbool.h>, if available. Performance things. Definitely avoid the mod operator. In this case, given your divisor is a power of 2, @harold's suggestion is great. In general, you can the same effect (every N'th item) with a counter: if (--remaining > 0) { remaining ...


2

General Observations This code should work just fine, but performance can be enhanced. Initialize Arrays to Zero at Compile Time Currently there is a performance hit in the code when initializing the array t to zero. The function memset() is a runtime function, by initializing the array t to zero there is no run time cost: char t[32767] = {0}; The ...


0

Just a few points for now, I will extend my answer over the weekend: I think you should think about the max size of your table anyway. I mean you can't have 4 billion nodes in there. This is connected to the size. If you only have to resize your table maybe once or twice, it's probably not worth storing the hash value. It's a trade off between speed and ...


9

Bit-reversal permutation bug Keeping both a normal counter and the "reversed counter" is the right idea, but this implementation isn't quite right. For example, it might result in a sequence such as 0, 4, 2, 6, 1, 3, 5, 7 while the correct one is 0, 4, 2, 6, 1, 5, 3, 7. A B 000 000 100 100 010 010 110 110 001 001 011 101 <<< 101 011 <&...


1

In addition to the other answers: strpbrk or strcspn are useful for finding the next delimiter in a string. strspn can be used to find the next character that isn't a delimiter in a string. So given a char const* str as input, we could do something like: while (*str) { char const* end = strpbrk(str, delimiters); if (!end) end = strchr(str, '\0'); /*...


8

Missing const I see you sprinkled const almost everywhere. However, you actually missed the one spot where it actually matters most: input should be a const pointer: _Bool fft(const complex_t* input, complex_t* output, const unsigned int size) { ... } Use the restrict keyword if possible Since input and output are of the same type, they can alias. This ...


3

compare is a dubious name. The purpose of the function is to tell whether parse string contains a character. Consider bool contains(char * str, char ch); The special case does not check for a possible overflow. temp_matrix_length could reach temp_matrix_size by the time it is executed. I do not endorse a pointer notation here. string[i] is easier to read ...


5

Major remark of the code as whole: for some reason you write the unreadable version of array access *(arr+i) all over the place. Don't do that! This makes your code look needlessly obscure and hard to read. Instead use the much more readable arr[i]. Your compare is just a naive implementation of strchr. It would be much more efficient to use strchr. Don't ...


2

In my testing (using C++ std::sin(), which should be the same function) the Standard Library sin() is about 9 times faster than the float version of LUT sine shown in @user673679's answer. If approximation suites you, as suggested already, You might find some help in Faster Math Functions (part 2) by Robin Green (part 1 also available). Here are couple ...


26

Your implementation is going to be slow, and the excuse "I need it to take a fixed amount of time" does not justify this. Using plain tables smells like cargo culting as well. So I'm not tackling what you did wrong in your code, but what you did wrong in even thinking about your implementation. First, google how to implement cosine on a micro ...


2

The problem is line *(words+j) = &str;. It says that we want words[j] or *(words+j) to point to where the str is pointing. So, at the end of the while, all the words are pointing to the same str with the content of red. It can be solved using strcpy method from string.h header, which copies the values not simply change the reference. strcpy(*(words + j), ...


4

The code is easy to read and to follow. There isn't a global in sight, the indentation is consistent, you use EXIT_SUCCESS and EXIT_FAILURE, and the control flow is simple. Therefore I have no comments about superficial aspects of style. However, there is a bug in interpret: You are reading uninitialized memory. The code assumes that code will end in a null ...


5

One remark is that you should get rid of all the needless branching and code repetition. It's bad for performance and code maintenance both. Given an angle you should be able to: Take it's absolute value. Divide by PI/2. Convert to unsigned integer, truncating decimals. Then you'll either have an index from 0 to 3 or you started with an angle larger than 2*...


5

#define PI 3.14... could use a few more digits! sine should work for numbers greater than 2.0 * PI. sine should work for negative numbers. same for cosine. if(temp > 2*PI) { temp -= 2*PI; } is ineffective for numbers greater than 4.0 * PI. if(rem > 0){ // sine value for given argument isn't directly in the lut if(index == (TABLE_SIZE-1)){ ...


1

Unfortunately, it looks like your code won't work. The last condition: if(butt3 = HIGH){ will always be true. This is a common mistake in C and C++, missing one = in a condition, turning it in to an assignment. A useful trick to avoid this is always put the constant first, then the compiler will catch the error for you: if (HIGH == butt3) { Another issue ...


2

Answers to your questions Should the main be broken into more functions? Yes. Keep it as high level as possible. Basically, it should look like: int main() { char sentence[...]; while (fgets(sentence, sizeof sentence, stdin) != NULL) { processSentence(sentence); } } Where processSentence() would do everything necessary to process a single ...


1

Domain-specific types Instead of char gender[6]; if you're intent on constraining to two genders, you're better off making an enum. However, it's in general a poor choice to do this; instead just accept a free string and don't bother writing logic for M/F. Pre-existing files It seems the only useful role that n_r has currently is to decide the write mode of ...


1

Answers to your questions I'm currently mostly looking for: 1) Readability, Macros make things less readable, both for humans and for source code editors. It is a necessary evil if you still want this kind of somewhat type-safe dynamic array in C. This problem is fixed in C++, where you can just use std::vector. Correctness, See below for some ...


1

Use consistent indentation I don't know if the indentation style of the code you pasted here is as you wrote it, but if it looks the same in your code editor, you should definitely try to fix it and make it more consistent. I would not bother trying to fix this manually, instead use either code formatting functions of your editor, or use an external tool ...


1

Split your program into multiple functions You can improve the readability and maintainability of your program greatly by splitting it into multiple functions that each do a simple, well-defined task. For example, you can reduce main() to: int main() { FILE* f = fopen("../input.txt", "r"); int numberOfValidPassports = ...


1

You were so close! So close to having re-entrant code. You declare buffer as a global, but also pass it into a function. It should be easy enough to just move it to main and keep your functions as-is. You have two different pointer offset styles (right next to each other) that accomplish the exact same thing: &tmp[tmp_offset], buffer+buffer_offset I ...


1

In addition to @Mark Bluemel good answer: Overlap A difference between memcpy() and memmove() is the ability to handle overlapping buffers. Notice the keyword restrict. void *memcpy(void * restrict s1, const void * restrict s2, size_t n); void *memmove(void *s1, const void *s2, size_t n); restrict roughly implies access to the buffer is not interfered with ...


1

Apologies for changing the code that was answered. Since a few days ago I have evolved :) Yes, that function is ok and will work for any legal call and that macro will work only with two string literals, of course. The only change I want to add inside the function is to call strncmp() instead of reinventing it // two char arrays comparison int safer_strcmp_ ...


2

Much of this feedback overlaps with that of @JakobLovern who has already covered some great points; nevertheless: You should break up main into subroutines You can avoid play being needed if you have a subroutine that returns a boolean Avoid aggressive abbreviation of your local variable names Consider adding an input validation loop Move your 'Goodbye' to ...


3

It looks like you haven't built this with compiler warnings enabled. That will give a lot of useful improvements (as will running under Valgrind). I compiled with gcc -std=c17 -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds -Wstrict-prototypes -Wconversion, and that identified a number of problems. #define ANSI_COLOR_RED &...


1

It's quite presumptive for a function to exit the program like this: if (!new) { fprintf(stderr, "Error: memory allocation failed\n"); exit(EXIT_FAILURE); } I'd argue that the caller is also better positioned to know whether an error message is useful, too: if (!new) { return new; } Similarly, ...


2

I also followed [Torvalds'] coding style I'm all for coding style standards, though I decline to listen to all of his suggestions simply because he's the loudest (seriously, very very loud) voice in the room. His blind obedience to 1988's K&R with no other rationale I find short-sighted as well. Anyway, enough editorializing: Your non-parenthesized ...


3

Input overrun option is one character; so why do you write 3 here? fgets( &option, 3, stdin ); Implicit string concatenation printf( "\n\t\t***Contact management system***\n1. Add new contact\n2. Show all contacts\n3. Show specific contact\n*Press 'q' to exit*\n" ); is more legible as printf( "\n" "\t\t***...


Top 50 recent answers are included