New answers tagged

1

Re-use could include stdlib so as not to have to define EXIT_SUCCESS myself, but as I only needed this constant, it's a little clearer and more efficient to do this Consider someone uses your good code as part of a larger task and does include <stdlib.h>, then the compiler may warn about the redefinition. Now the next coder needs to spend time ...


1

If you can include the appropriate header, do that. Redefining yourself is error-prone even where allowed. This is C not C++, headers are simple enough it doesn't significantly influence compile-time. If you add the origin of a constant in a comment, try to instead let the compiler determine it from the true source for you. char result[9] = ""; /...


11

#define EXIT_SUCCESS 0 You are not permitted to define standard library names yourself. So just include <stdlib.h>, or omit the return from main() (remember, C will provide a success return if you don't provide one). void fizzbuzz(int n) { We could give that internal linkage: static void fizzbuzz(int n) { char result[9] = ""; // strlen(&...


3

It looks pretty good to me. I still have some notes to make. Let's start with, there are 5 arithmetic operations (per row) to do the work that 4 FMAs could do. How good or bad is that? Well basically not bad at all in the context of just a simple 4x4 multiplication, because there are also 8 loads for those 5 arithmetic operations, so the problem is the loads....


3

Broken corner cases "The first function will always try to correct the final result, " is unsupported, due to undefined behavior, when mod + b overflows as in floor_mod1(1, INT_MAX). I think that in general the second function will be faster, Speed should be a secondary concern, get the function to work correctly for all int a, int b. Make code ...


4

You can use Godbolt to play around with the C code and see if anything makes the assembly look particularly better. (Seems like Clang is strangely much better than GCC at making this code branch-free.) Technically, (a ^ b) has implementation-defined behavior if you end up XORing non-zero sign bit(s) together. It's better (in terms of portability) to write (a ...


3

if I can make some improvements to it or if there are any error. Lack of error checking fopen(), malloc(), ftell(), fread(), (all I/O functions), etc. deserve to have their return values checked for errors. In particular when a file may fail to open or memory fail to allocate, handle such cases to not forget the other. e. g.: do not forget to fclose(), or ...


2

should I worry about dealing with overflow in the computation of ord? Yes. The difference may overflow. Instead compare: // Compute cross product sign (see next bullet) int64_t ba = b[0]*a[1]; int64_t ab = a[0]*b[1]; if(ba < ab) return -1; if(ba == ab) return 0; return 1; can I simplify this comparison function even further? Excessive comparing is done. ...


1

Early returns You have a bunch of them. I'm all for them (not everyone is), but you're inconsistent in your else style after an early return. Sometimes you do this: if(a[1] == 0){ return a[0] > 0 ? 1 : 5; }else // ... and sometimes you dispense with the else: if(a[1] > 0){ return a[0] > 0 ? 2 : 4; } return a[0] &...


0

Make your code more generic What if it's not only Andy, Amy and Benson, but you want to store the height and weight of a thousand people? Maybe some share the same first name? You can imagine that this would result in a terrible mess. The first thing to do is to write your code in a generic way, so it can handle any number of persons without having to ...


3

Your chmod needs to go away; d.sh should already have the correct permissions, including when extracted from a source archive You should probably delete all of your output diagnostic fluff. f.c and d.sh are very bad filenames. The former should probably be renamed to fractal.c or somesuch. You've mis-used make. The whole point of make is to express a ...


5

Memory management and optimization is important. I will avoid any stylistic remark and focus on this particular aspect of your query. Memory First of all, the first rule of optimization is to avoid allocating memory. In general, memory allocation is slow-ish, and memory de-allocation even slower. In particular, most malloc implementation will perform "...


5

You have used static for the helper functions, to give them internal linkage. That's good. return (1); The parentheses don't add any value here. Just write return 1; like everyone else. We can avoid the need to special-case n == 0 by always performing at least one division; use do/while in place of while: int i = 0; do { n /= 10; ++i; } ...


4

Leak digits is never free'd. If either one of digits or nbr return NULL, the other allocation is lost. digits = getdigits(n, digits_size); ... nbr = malloc(digits_size + 1 * sizeof(char)); if (nbr == NULL || digits == NULL) return (NULL); Concept error sizeof(char) is 1 so not much of a issue, yet sizeof(char) should multiply by the sum. // malloc(...


7

Besides lots of optimizations other people have suggested, you have two memory leaks. The first is that, just before the final return in itoa(), you need to write free(digits); The second is in the error handling. (A common place for errors BTW.) In particular, if getdigits returns successfully, but the malloc fails, you leak digits. The fix: Test digits ...


3

The standard functions in string.h are not using malloc and free almost all. Therefore, the interface of itoa might be changed like this: /* NG - because itoa returns a new memory */ char *itoa(int n) { ... } /* OK - because itoa uses an existing memory */ void itoa(char *s, int n) { ... }


9

Counting the digits doubles the amount of work, and the division/modulo is indeed very hard work for the CPU. You know the maximum size of the resulting string, because it would be the largest or most negative integer you can be passed. So use a local array as a buffer, and then copy the result to allocated memory. You have a separate function to reverse ...


1

The obvious issue with this code is the lack of structure. There are two main computations, given that we've already spotted that we can add all the exponents and perform a single exponentiation with the sum: Finding the Lth and Rth odd-length palindromic numbers Modular exponentiation Each of these should be separate - independently testable - functions; ...


3

Because you test for SPM_UNIX_ first. That test includes test for the value of: || defined(__MINGW32__) \ || defined(__MINGW64__) \ || defined(__CYGWIN__) \ || defined(__CYGWIN32__) If any of these are true then you have a SPM_UNIX Which means that these tests: && !defined(__MINGW32__) && !defined(__MINGW64__) \ &&...


3

Brute-force searching is severely inefficient. Try for a better algorithm by observing the problem carefully: Algorithm: Finding a way to easily find the n-th smallest palindrome number begins with a basic question: How many are there for a given number of digits? digits count of palindrome numbers 0 none (or does zero count?) 1 9 (or 10 with zero), 1-...


1

Forget about code (until the very end) and just consider how you could generate a sorted list of palindromes (of odd length) "by hand". The first (1-digit) palindromes are 0, 1, ..., 9 i.e. the palindrome at position n (0 ≤ n < 10) in the list is n. After that come 101, 111, 121, ..., but finding them by hand gets tedious (especially since the ...


1

pow(double _base, double _exp), like this pwr(), is a challenging function. Also an accurate implementation is difficult as errors in manipulating _base are heavily amplified by large _exp. Non-integer power When _exp is not a whole number value, best to use exp2(_exp * log2(_base)) and avoid log(), exp() to improve accuracy. Note: _base < 0 will error as,...


1

It's very heavy on macros, but that's inevitable when we provide functions to extend the language in this way. Users don't have to look at the full gory details anyway, and the user interface looks good. I like the consistent prefix, which makes it very easy to see at a glance that they are related. It would be instructive to show a function that accepts a ...


2

int is_palendrome(char *arr,int n,int l,int r) Spelling: palindrome Consider including <stdbool.h> and returning a bool value. The arguments could do with more descriptive names. It's particularly unclear what n is for, as it appears to be unused within the function. Why does arr point to modifiable char? I think we should pass const char*. l and r ...


2

Description "Finds largest palindromic sub-strings in a string without repeating any character"? Sure? Maybe "splits the string into the palindromic substrings"? Because, say, "ababba" would be spited into "aba", "bb", "a" - not "a", "b", "abba" to have the largest ...


5

The interface There are a number of useful things to return. Let's list them in order of which is least surprising: The pointer to the start of the destination. This follows the standard library, but is least useful. The pointer to the end of the destination. This follows an alternative convention embodies in the stpcpy() function and similar added to ...


2

First impression is pretty good - care taken with exception conditions and thought to the interface. Headers: I find it helpful to have a consistent order for headers, to quickly see if the header I need is already included. For this, I group the Standard Library headers in alphabetical order. You might find that helpful too. char *ptr = strstr(src, orig);...


1

For exp not 0, between -1 and 1 it will make a recursive call with a negated exponent. Furthermore x²ⁿ = (xⁿ)² which can be used to reduce n recursions down to log₂(n) recursions. For exp = n + q, n int, 0 < q < 1 or exp = n ± q, n int, 0 < q <= 0.5 you can use xexp = (xn).(xq). For the integer power n you can do the logarithmic recursion (double,...


0

I've modified my code according to the remarks that have been made here. Changes include: Declaring outside of the loop variables that aren't dependent of it (see screen_height and screen_width) Using binary operators to get rid of the if inside the nested for loops, by using a mask, which is an array of the same dimension of the sprite sheet, to know if a ...


1

For large exponents, this code is slower than necessary (it scales linearly in exp, rather than logarithmically as an efficient implementation would). Research Binary Exponentiation algorithm, and Exponentiation by logarithm, which is what your standard library implementation is most likely to use for pow(double, double). The latter relies on the identity \$...


7

There are a few things that can be lifted out of loops. The three screen related variables, screen_height, screen_width, and screen_size, along with scanline_in and scanline_out, can be initialized before the outer i for loop. In the main blit loop, the check for scanline_out_index <= screen_size can be moved to before the x loop (and when false, you can ...


2

main() void main() Oops: that's not a standard signature for main(). You probably meant int main(void). We have a memory access bug here, that's illuminated when we run under Valgrind: char arr[10] = "abcdeabcde"; int n = strlen(arr) - 1; arr doesn't have a terminating null. It's better to use an array of unspecified size, so we can be ...


9

it seems to be optimized quite well by the compiler, as it uses a lot of vectorized instructions Well, kind of. GCC didn't use vectorization at all, and Clang used some of it but in a very strange way. Let's look at what it's doing. Basically I'm reviewing what Clang did, not so much your code.. but that then informs you about your code by proxy. I'll go ...


0

What's <conio.h>? It's not mentioned in the C standard, and if I remove it, the code kind of compiles, so I guess it's not required. I say "kind of" because of the warnings it generates, which shouldn't be ignored: gcc -std=c17 -fPIC -g -Wall -Wextra -Wwrite-strings -Wno-parentheses -Wpedantic -Warray-bounds -Wstrict-prototypes -...


3

Weird enum values Why does stmfrm_state start at 0x8000, and then goes down? Also, if the enum names all start with SMFRM_WAIT_AA and only have a different number at the end, this tells me you shouldn't be using an enum here to begin with, and instead you should use an unsigned int counter. Indeed it looks like it's just counting how many bytes of the header ...


1

How I Review Code I have performed Code Reviews for over 30 years both on the Code Review Community and professionally. I have also had to maintain code written by others and this generally requires a thorough code inspection before starting. The criteria I use for code reviews is: Does the code compile without errors and warnings? Does the code work as ...


2

You have a great review by pacmaninbw, but I'll just pick up on one aspect and take it a little further. There's some quite involved machinery for printing error messages: struct data_structure_errordesc{ const int32_t code; const char *message; }; const struct data_structure_errordesc errordesc[] = { {E_SUCCESS, "No error" }, {...


3

How I Review Code I have performed Code Reviews for over 30 years both on the Code Review Community and professionally. I have also had to maintain code written by others and this generally requires a thorough code inspection before starting. The criteria I use for code reviews is: Does the code compile without errors and warnings? Does the code work as ...


4

You have a thorough review already; I'll just add some minor points. We have a pointer to char in the cell structure: typedef struct lv_cell { // The text in the cell char *text; … } lv_cell; However, we don't need to modify the contents of the string, and making it const char* (and updating function signatures to match) would allow us to pass ...


Top 50 recent answers are included