New answers tagged

1

Only for the pedantic With non-2's complement, the below will compare +0 as matching -0. char* addr1; char* addr2; ... if(addr1[i] != addr2[i]) ... // Oops. +0 == -0 Instead use unsigned char * unsigned char* addr1; unsigned char* addr2; ... if (addr1[i] != addr2[i]) ... // OK. Only one kind of 0 Better yet, use memcmp(). Return values "... returns 2 ...


2

You can multiply the adjacency-matrix n times to get all paths of length n between any nodes. Therefore, if you multiply A^n * vector_with_only_your_startVertex, you can see in the resulting vector which nodes can be reached. Make sure to also safe nodes that can be reached in less steps, because for example there may be cases where a node may be reached in ...


1

A few things I noticed: You have a double include of sys/stat.h When checking through value of argc, instead of saying “Something wrong with variables”, it would be better to print a notice that the user gave the wrong number of arguments, along with a small usage statement. There’s also an error code for that case, I believe. You set the file size ...


1

Here are some things that may help you improve your program. Use standard library calls where appropriate Instead of your for loop, I would recommend using memcmp() to accomplish the same thing, but likely more efficiently since the library version typically compares more than one byte at a time. Define variables where they are declared The addr1 and ...


1

In addition to remarks by previous reviews: This is wrong: int main(const int argc, const char *const *const argv) The form of main() is for the compiler to decide, not the programmer. This form is not at all compatible with standard C int main(int argc, char *argv[]). So unless your compiler docs specifically tell you that your custom form of main() is ...


2

In larger programs, lots of "compiler switches" (#ifdefs) make the code very hard to read, "clunky" if you will. It's not an issue in a tiny program such as this, but for larger programs you should consider a more "polymorphic" approach. Like for example having a generic "epoch.h" as the platform-independent API. Link this with with a corresponding "...


4

Here are some things that may help you improve your code. Fix the bug The program starts reading the header as though the file pointer were already pointing to location 0x100. That's an error because in all of the Gameboy files I've ever seen, the first 0x100 bytes are present and needed for calculating the checksum. Use a struct where appropriate This ...


4

Using static file-scope variables static struct timeval lnm_current_time; // ... static FILETIME lnm_win32_filetime; makes your code thread-unsafe: Two “simultaneous” invocations of your function from different threads access the same memory. (For other potential drawbacks of static file-scope variables see for example Are file-scope static variables in C ...


4

C99, assignment-in-conditions At the risk of sounding like a broken record - I'll make the same recommendations as in Simpletron simulator in C . Consider moving your variable declarations closer to where they're used, and expanding out your assignment-in-condition statements. There's another benefit to C99: this - comp->symtable = symtable; comp->...


1

Here are some things that may help you improve your code. Fix the bug The code allocates memory for the passed arr_lengths but never returns a pointer to the newly allocated memory to the caller. The code currently has this: int* lengths = *arr_lengths; // more code (lengths) = malloc((max_dis+1) * sizeof(matrixOptimizationValue)); Instead it should be ...


4

Wrong compare fgetc() returns an int with the value of EOF or something in the unsigned char range. Although EOF is commonly -1, it is not specified as so. int c = fgetc(fp); // if(c == -1) if(c == EOF) Useless cast The 2nd cast is not needed as c is in the unsigned char range so there is no value change. Before the ~ is applied, the (...


2

the posted code is ignoring the returned values from scanf() fscanf(), and system() Critical success/fail information is contained in those returned values, so the code should be checking those returned values Please read why while(!feof(ed)) is always wrong suggest replacing: while(!feof(ed)) { fscanf(ed, "%d %f ", &list[*loc].empid, &...


6

Error printing You do the right thing in some cases: if(fseek(fp, -2, SEEK_CUR) == -1) perror("fseek"); but not others: if(fgetpos(fp, &pos) == -1) return -1; Also, that particular check does not adhere to the specification, which says: Upon successful completion, fgetpos() shall return 0; otherwise, it shall return a non-zero value and ...


3

Very nicely code. Only some nits. Help "I wrote a manual page for it" --> Perhaps it, or a condensed version for option -h? case 'h': printf("blah blah\n); exit (EXIT_SUCCESS); > vs < Conceptually, when looking for "smallest values", I'd like to find a <. Perhaps: // if (roll[i] != 0 && min > roll[i]) { if (roll[...


4

In addition to the review you already have, I have a few more suggestions. Fix the bug As was already pointed out, the assignment-in-condition practice is problematic. In this particular case, the problem is in main. The current code has this: if ((memory = calloc(memsize, sizeof *memory)) == NULL) err(EXIT_FAILURE, NULL); if ((fp = fopen(*argv, ...


1

Is it well commented? Generally yes, the only place I might add more comments is to explain the fields in the structure. Is my solution portable? No, it will not port to Windows easily because of the use of libbsd and the use of the unistd.h header file. To improve portability it is also important to get comfortable with the memory allocation functions ...


7

Re-entrance These: static int *memory; static int acc; /* accumulator register (value being processed) */ static int ireg; /* instruction register (current instruction) */ // ... static int count; static int opcode; static int operand; force a user to start a new program if they want a new instance of the calculator. If you want to ...


3

OP: Please leave any feedback you see fit, with special attention to the loops and data types. char type Although char is signed or unsigned, string functions of the standard library work with the data as if it is was unsigned char. ... each character shall be interpreted as if it had the type unsigned char ... C17dr §7.24.1 3 Also: char output = 0; ......


1

Using 1201ProgramAlarm's comment, the code ran slightly faster but was still slower than taking multiples of 3(effectively 6). However finding if the current composite number is in the multplicative group mod 30 seemed to be the one that took up a long time and could have been memoisation. This is done by precomputing how many times one adds the prime to ...


2

There are no classes in C. However, there are functions. Regarding: CFLAGS = -g -O2 -Wall $(MACHDEP) $(INCLUDE) When compiling, always enable the warnings, then fix those warnings. ( for gcc, at a minimum use: -Wall -Wextra -Wconversion -pedantic -std=gnu11 ) the = will cause this macro to be re-evaluated every time it is referenced. Suggest: ...


1

O(n*n) With a changing str and strlen(str) in the for(i = 1; i < strlen(str); ++i) loop, code repeatedly calculates the string length. Once is enough. Even easier, test for the null character. is...(int ch) called with char is...(int) and to...er(int) functions expect an unsigned char value or EOF. When called with a char less than zero (and not EOF)...


3

When the original multiples-of-3 code finds a prime, it starts setting bits with the square of that value (for (int j = i * i). Your multiples-of-30 code does not do this, and can waste a lot of time marking numbers "not prime" that have already been so marked. As the new prime gets larger, this will consume a growing amount of time.


6

Statics You have a one-file program, so more of your methods and globals, including these: int playerLossesCount[NUM_PLAYERS]; Wall walls[NUM_PLAYERS][NUM_WALLS]; GXTexObj texObj; should be made static. Declarations for variables int main( int argc, char **argv ){ u32 fb; // initial framebuffer index u32 first_frame; f32 yscale; u32 ...


2

In the context of function parameter passing, char* and char[] are equivalent (because char[] decays to char*). Anyway, I would rather use title_case(char* s) because it more clearly expresses the truth: what you really get within the function is a pointer to the first character of the array. Accepting char* as string parameter implicitly suggests that the ...


5

int generate_random_number(int min, int max) { srand ( time(NULL) ); return min + (rand() % (max - min)); } You should only seed the random number generator once, at the start of main. In this case, if (for some reason) somebody played more than one round in a single second, both games would have the same number. Probably not a huge issue for this ...


3

if (difficulty == 1) { else if (difficulty == 2) { else if (difficulty == 3) { This looks like switch (difficulty) might be more appropriate - perhaps with a default branch to catch out-of-range values. Or, more simply, since we're just picking values, and min is always 0, just select from an array values (after verifying that the user's choice is in range)...


5

Good: no warnings with -Wall -Wextra -pedantic with both gcc and clang, no memory leaks found with valgrind. Bad: use prototypes instead of declarations to give compiler a chance to issue warnings when an incorrect number of parameters is passed or incorrect types are passed: void play(void) int main(void) You don't check if user passes correct ...


2

The storage of both points and queries is suboptimal - in both cases, they are plain unsorted arrays. We could use better strategies for representing one or both. As a simple example, consider keeping points as a list of rows in ascending order, with each row being a list of points. Now, when we evaluate a query, we can quickly skip the rows that are ...


1

Datatype The size_t size is appropriate, but int top is less. This effectively limits an entire size_t to just storing INT_MAX, and the only value that is negative is -1, the rest are wasted. gcc gave me a warning about comparing integers of different signs, but they should moreover be the same type because they are both storing indices; instead of size and ...


3

I'm interested in constructive feedback on how I could improve the algorithm to make it faster and do less checks. processQuery() is O(n) with for(size_t i = 0; i < npoints; i++) { An alternative would create a binary like tree in 2 dimensions. Not a BST, bit a quadtree. Then the searching within a rectangle could take advantage of potentially O log(...


3

A small version compare improvement would perform up to n +1 rather than 2n compares. Instead of if(pL->major > pR->major) return LEFT; if(pR->major > pL->major) return RIGHT; if(pL->minor > pR->minor) return LEFT; if(pR->minor > pL->minor) return RIGHT; ... Compare for equality first: if(pL->major != pR->major)...


11

Don't strtok + atoi. Use strtol, which (a) doesn't need a mutable input, (b) has much better error handling and reporting, and (c) eliminates the need for independent validation. An example of use would be char * end; pVer->major = strtol(str, &end, 0); if (*end != '.') { // major is not a number. return suitable_failure; } str = end + 1; ...


7

Instead of spelling out every single element of the array, char ver_string[] = {'5','.','2','5','.','5','.','0','\0'}; just use a string literal. It's the same thing: char ver_string[] = "5.25.5.0"; Should I be printing to stderr only in the driver (main) program and leave all of that out of the main "library" code? Yes, exactly. You can return error ...


5

The validation of the string seems to leave some gaps. For instance, the version numbers seem to be valid whatever size they have. They could be zero digits large, or contain so many digits that they would not fit into an integer. I'd try not to print to standard error if this is supposed to be used as a library. In that case you might want to use separate ...


2

I would tag your struct in addition to, if not instead of, using a typedef: struct v2 { int x, y; }; // typedef struct v2 v2; Then you can use struct v2 wherever you are using v2. This makes it clear that v2 is a structure. I think you should create a symbolic SUBGRID_SIZE instead of using 3 here: x0 = (pos.x / 3) * 3; y0 = (pos.y / 3) * 3; for (i = ...


3

You could allow to initialize the stack locally. Just by separating the bigger part of stack_create to separate function. void stack_init(STACK* stack, char* mem, size_t size) { stack->mem = mem; stack->size = size; stack->top = -1; } char stack_mem[STACK_SIZE]; STACK stack; stack_init(&stack, stack_mem, STACK_SIZE); As for error ...


3

Separate pin definitions Clearer and easier to maintain. //#define SLAVEPINS 8, 9 //SoftwareSerial serial(SLAVEPINS); // RX, TX #define SLAVEPINS_RX 8 #define SLAVEPINS_TX 9 SoftwareSerial serial(SLAVEPINS_RX, SLAVEPINS_TX); Code guards #ifndef _SLAVE_H #define _SLAVE_H 1 #define SLAVEPINS 8, 9 #define SLAVEEVENT 200 .... #endif Also see #pragma ...


1

Spaces My first read of this code: "The \"test\" command, as well as the \"[\" command, are not required to know", "the \"==\" operator. Only a few implementations like bash and some", "versions of ksh support it.", was wrong; I didn't notice that it's actually a varargs-function accepting one line per argument. That's a little odd. This has ...


3

Use a common protocol file Any common constants, such as 9600 SYN, ACK, etc. strings should be #defined in one place, in this case a .h file accessible to both Arduino projects. Indentation One-space indentation is essentially never used. Typically four is used instead. Globals In the master code, every single one of those globals should be moved to ...


1

it is correctly implemented (?) No. Incorrect result when n <= 10. Goal: Counting the number of digits with a recursion algorithm in c Simply change the condition to handle all int. int countDigits(int n){ // if(n>=0&&n<10){ if(n > -10 && n < 10) { return 1; } else { return 1 + countDigits(n/10); } }


1

in this function: void list_remove(List* head, int index) there is the statement: head = head->next; is a problem as this changes the parameter on the stack, not the actual list in function: main() this statement: List list = *array_to_list((int[]){1, 2, 3, 4}, 4); will result in the variable list containing the contents of the first instance of ...


3

I unfortunately cannot comment (not enough reputation), but in terms of performance and generated assembly code both variants are exactly same: int countDigitsSane (int n){ int digits=1; for(int i=n; i>9; i/=10) { digits++; } return digits; } int countDigits(int n){ if(n<10){ return 1; } else{ return 1+countDigits(n/10); ...


2

As mentioned in comments, you should be careful about which integer type you use. int is negative and if your function takes int as parameter, it will therefore be assumed that it can handle negative numbers. So unsigned int might have been a better choice. More importantly, you should be aware that recursion is dangerous, ineffective and often hard to read....


1

The consensus from the never-ending goto debate is pretty much that "yeah there are cases where you can use goto harmlessly when branching non-conditionally downwards, but those cases could as well be written without goto too". So your code isn't horrible, but it isn't pretty either - it looks like the old "on error goto..." error handling pattern used by ...


3

Stack* helper (char val, int x, int y) { Stack* node = (Stack*)malloc(sizeof(Stack)); node->v = val; node->x = x; node->y = y; return node; } That's not a very descriptive name - something in keeping with the existing scheme might be stack_node_create(). Since malloc() returns a void*, there's no need to scare readers by ...


8

goto PAGE_FAULT; PAGE_FAULT: This code seems almost like a troll. You can certainly rewrite it to use no gotos; all you have to do is un-flip the conditions that you must have flipped when you inserted the gotos in the first place. For example: if (frame != SENTINEL) { goto PAGE_HIT; } pageFaults++; frame = loadValueFromBackingStore(...


4

Consider separating the stack from the stack entries. Right now, you have to pass a pointer to a pointer to the stack to your push & pop routines. Quuxplusone’s solution requires the caller to do the work of assigning the return value to the stack pointer. Both of these are harder to use. Moreover, with Quuxplusone’s solution, you can’t pass an empty ...


4

Stack* helper (char val, int x, int y) { Your indentation and whitespace are kind of funky; I recommend looking at what some popular open-source code on GitHub does, and trying to copy them as closely as possible. Or, just run your code through a formatter such as clang-format. Stack *helper(char val, int x, int y) { This function is only ever used in ...


Top 50 recent answers are included