18
Welcome to Code Review, we wish you the best.
General Observations
Congratulations on getting this to work, one function that is 356 lines long and almost 17K of text is a bit large and very hard to code and debug.
Complexity
The function main() is too complex and should be broken into functions. A general rule of thumb in all programming is that a function ...
11
Please check it if you can, and give your opinion!
Enable more warnings
warning: this statement may fall through [-Wimplicit-fallthrough=]
unused variables `int a, b, i;`
Spell check
funtion
Do not re-enter main()
Yes, it is possible, yet makes code review and debug a bear and hard to spin this code off into its own TicTacToe() function. Don't do that.
...
10
Making shebang line more portable
Fixing the perl binary to /usr/bin/perl in the shebang will not work if you are using perlbrew instead of the system perl. Using /usr/bin/env perl will be a more portable alternative.
use strict and warnings pragmas to catch error at an early stage
By adding warnings you will get quite a few warnings, as discussed below.
...
8
Missing Error Checking
I usually use c++ so it may not be best practice for c.
In C++ when memory allocation fails in new an exception is thrown, this is not the case in the C programming language when using malloc(), calloc() or realloc(). An additional check is required after any memory allocation call. The check is to see if the memory returned is NULL ...
7
General Observations
The code displays some good programming habits already, such as include guards, good indentation, and wrapping all in if statements, else clauses and loops in braces ({ and }). The file structure is good and it is easy to find which files need to be modified in maintenance. Many of the private sub functions are already removed from the ...
7
Here are some things that may help you improve your code.
Fix the bug
The code currently includes this line in initialPosition:
*(emptyBoard[i] + j) = (bool) (startPos[(i * width) + j] - '0');
Since each row is width cells wide, we should be multiplying by j rather than i.
*(emptyBoard[i] + j) = (bool) (startPos[j * width + i] - '0');
Eliminate ...
7
Here are some things that may help you improve your program.
Use only required #includes
The code uses nothing from <stdlib.h>, so that header can and should be omitted. Only include files that are actually needed. This makes the code easier to understand and maintain and also may slightly speed up compiling.
Try to write portable code
Things ...
7
Use getaddrinfo only once, to resolve the host. Do the port number iteration directly yourself. While it might be more complicated, it also doesn't call for repeated hostname resolutions. This also allows you to eliminate convert_int_to_string()
If you don't want IPv4 and IPv6 (which seems to be indicated by specifying AF_INET, you might be better off ...
6
Alignment
This will be an easy win - use aligned_alloc instead of malloc. This is only guaranteed to be available in the standard library as of C11, which you should be using anyway.
Exponential reallocation
This:
// gracefully extend buffer size
nofs++;
buf = realloc(buf, BUF_SIZE*nofs*sizeof(char));
reallocates with linear growth. ...
5
The code almost works.
To make it work in all cases, test the program with Valgrind, which detects undefined behavior because of invalid memory access. This will prove that the buffer needs to be 11 bytes long, not only 10.
What about platforms where int has 64 bits instead of just 32? For these you need a larger buffer. Until then, you should use a compile-...
5
Along with the excellent answer of Håkon Hægland,
Use perl's quoted regexes to simplify your code and improve readability massively.
my $keyword = qr/[A-z][A-z0-9]*/;
my $bits = qr/(?:8|16|32|64)/;
Makes:
s/entry ([A-z][A-z0-9]*)\s*,\s*([A-z][A-z0-9]*)\s*:/int main(int \1, char* \2\[\]) {\n\tSDL_Init(SDL_INIT_EVERYTHING);/g;
s/([A-z][A-z0-9]*)\s+:u(8|16|32|...
5
Variable linkage
bb first;
looks like it's in a header file. That means every time you include it from a different module, it will be re-declared with its own separate address. That's unlikely to be what you want. Instead, declare it extern, and then define it once in a C file.
Beyond that, though: why declare it in the header at all? It's an implementation ...
5
Here are some things that may help you improve your code.
Be careful with array lengths
The memory allocation for the virtual machine is currently this:
i32 mem[0xffff]; /* approx. 64k */
While there is no intrinsic problem with this declaration or the comment, it means that memory locations 0 through 0xfffe are valid, but memory location 0xffff is not. ...
5
Many reviews, so some additional ideas.
Programmable precision
Rather than hard-code the 5, use a flexible variable. Perhaps code later may want to allow the user to adjust the width.
// printf("Here is your answer:\nLg(%f) = %.5f (to 5 decimal places)\n\n", Num1, ans);
int prec = 5;
printf("Here is your answer:\nLg(%.*f) = %.*f (to %d ...
4
... looking for help speeding up some code
Functional concern
I see no provisional to insure the allocation meets universal alignment as malloc() does. This is a potential fatal error. Also research max_align_t.
7.22.3 Memory management functions The pointer returned if the allocation succeeds is
suitably aligned so that it may be assigned to a pointer ...
4
I have a writeOnFile() method with too many fwrite(). ...
A way to reduce fwrite() calls is to simply write all text to a large char buffer[] and then write that once per for (int i = 0; i < global_index; i++) loop.
Use the return vale of snprintf() to speed calculation of next offset. Use *printf() features for padding.
for (int i = 0; i < ...
4
While harold reviewed your assembly code, I'll just comment on how you are compiling your C code:
Increase the duration of the benchmark
Your code runs for a very short amount of time, only tens of microseconds. This is too short to get accurate measurements:
clock() only has a resolution of a microsecond, so this is not insignificant compared to the ...
4
How could I do better using basic SIMD and SSE?
The most important things are:
Delay horizontal addition as long as possible
haddps costs two shuffles and a normal addition. It is used twice, plus a shift and scalar-add. That's a lot of cost, and none of it is necessary: the main loop can just use addps. When the main loop is done, then you still need ...
4
Here are some things that may help you improve your code.
Separate interface from implementation
It makes the code somewhat longer for a code review, but it's often very useful to separate the interface from the implementation. In C, this is usually done by putting the interface into separate .h files and the corresponding implementation into .c files. ...
4
When accessing array elements, don't use the clunky *(board + i) notation. Use board[i]. (You're already doing this in some places, and you should be consistent.)
Eliminate the use of global i and j variables as loop variables. Use local variables instead. This can help with optimization, and avoids problems where a function called while in a loop in ...
4
open to expansion
Avoid naked undocumented numbers
Code has SDL_Delay(500); 3 times. Are the values related? 500 what?
Since it is likely the values are to change in common, create code the reflects that. This makes for easier to expand/maintain code.
#define VIEWING_PAUSE (500 /* ms */)
// SDL_Delay(500);
SDL_Delay(VIEWING_PAUSE);
Likewise for 127,...
4
Whereas @pacmaninbw offers some excellent general strategies, let's look at some specific syntax.
Pi
This is a point of some contention, but - where a library defines M_PI, and most do - I tend to use it. Since you're including windows.h it's likely that you're using MSVC. In that case, it gives you math constants if you configure the compiler with ...
3
It is doubtful that the program is so long that it can't all be included, but you have made an effort to comply with the Code Review guidelines. Just be aware that comments such as // more logic here or // ... will sometimes get the question votes to close.
Complexity
You're a Java programmer so I'm going to assume you understand object oriented programming ...
3
General Observations
The code has improved from the last version quite a bit. It now utilizes a more common algorithm for getting primes and this new algorithm should perform better. The code in hashcons.c is less complex and most or all of any possible bugs have been removed (thank you for removing the possible recursion).
I have taken the liberty of adding ...
3
Undefined behavior: Access outside array
Allocation is for n elements, yet code attempts to access 1 past primesTable.table[n-1]
... malloc(n * sizeof(bool));
...
for (int i = p * 2; i <= n; i += p) primesTable.table[i] = false;
// ^
Bug, wrong function type
hash() returns long yet .hashf points to a function returning int.
long hash(...
2
IMO this kind of error prone, hard to maintain macros should be avoided at any price.
If I was doing it I would do something like this (assuming gcc):
typedef struct
{
size_t nelem;
size_t elemsize;
char arr[];
}DARR_t;
#define INLINE inline __attribute__((always_inline))
#define arr_init(nelem,type) _arr_init(nelem, sizeof(type))
static ...
2
Overall it looks fine for a beginner-level program, you keep the functions small and separate different parts of the logic between them, which is good.
int valid(int[][9], int, int, int); You should make a habit of naming your parameters even during function declaration. Ideally the function definition is a copy/paste of the declaration.
(Minor remark) It ...
2
I have one comment about for loops. According to standard, for loop condition is evaluated on every iteration. In function is_prime expression floor(sqrt((double)x)) will be evaluated several times which will cause performance penalty. It is better rewrite this loop. For example:
int condition = floor(sqrt((double)x));
for (int i = 3; i <= condition; i += ...
2
Code structure
Treat main.c as boss who calls other people up to do their job. In this case, functions where one function does one thing. main is doing everything here.
else{
printf("No. The correct answer is %.0d. Need to practice more!\n\n", c);
system("pause");
system("cls");
}
This error message can be one function which ...
1
Use standard array notation
When you are accessing array elements, the idiomatic way in C is to write foo[bar] instead of *(foo + bar). So instead of:
int next_number = *((i * rows + arr) + col);
Write:
int next_number = arr[i * rows + col];
It is both shorted, and now it is clear what part the array pointer is and what part the index is.
Avoid unnecessary ...
Only top voted, non community-wiki answers of a minimum length are eligible
