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;
...
10
I suggest that you do more error checking:
malloc or realloc may fail.
I can initialize the stack with negative capacity.
I can pop more elements than I pushed.
In all these cases, your implementation silently swallows the errors and accesses invalid addresses.
As πάντα ῥεῖ said in comments, if you want to store generic elements that exceed void* in size, ...
8
There are two main problems with the code that must be addressed:
You store pointers to data in an array, rather than actual data. This is not useful, since pointers may point to data that goes out of scope or otherwise becomes obsolete. When writing a container class such as a stack/LIFO, one should store so-called "hard copies" of the data passed.
The ...
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(...
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 ...
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 ...
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 ...
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 ...
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 ...
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 ...
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 ...
5
stack->data = (uint8_t *)realloc(stack->data, stack->capacity * stack->itemSize);
This is wrong, because if realloc() failed, then there's now no way to access the memory previously pointed to by stack->data. The correct pattern for realloc() is:
void *newData = realloc(stack->data, stack->capacity * stack->itemSize);
if (!newData) ...
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 ...
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 (...
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 ...
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)...
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);
...
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)...
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 ...
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 ...
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 ...
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.
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; ......
3
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, ...
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[...
3
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->...
Only top voted, non community-wiki answers of a minimum length are eligible
