15
(This didn't fit in the comment field, not really an answer, more like advice)
I get the allure of rolling your own format and writing the code, you wanna do it right, you wanna squeeze every last ounce of performance out of the CPU, you wanna create something awesome, maybe even learn something along the way, but here's some advice from a professional ...
11
Anyway, I want to get tips and criticism (Especially about my format).
I think the fixed-size 4-byte things are great, much better than text. They're easy to use both for humans and computers, the identifiers are easy to identify in a hex view, and nothing misaligns the data stream so it stays in nice columns in hex view as well. However, I don't think it's ...
10
More specific types are available for an eight bit number, such as uint8_t
static keyword is dangerous (when used as an output buffer) and results in weird behaviour if your method is called twice
The arithmetic can be replaced with a more simple & (bitwise AND)
void to_binary(uint8_t x, char *output) {
*output++ = '0';
*output++ = 'b';
for (...
8
fread advances the file pointer. You may safely discard all those
offset+=4;
fseek(file, offset, SEEK_SET);
When you really need an offset, call ftell.
Always test what fread returns. BTW, it would eliminate a need in maintaining offset and comparing it with fileSize.
float x, y, z;
fread((void*)(&x), sizeof(x), 1, ...
7
Make use of the standard library
You are reinventing lots of wheels in your programs. There is already a lot that the standard library you are using will do for you. Even in C on Linux, you can use POSIX functions for managing hash tables: hcreate() and hsearch(). But even better is probably to write both the Linux and Windows versions in C++, and make use ...
7
Bug
Code attempts to form a string. Yet it lacks a terminating null character and space for it.
// static char binary[10];
static char binary[11];
As a static, it is initialized to all zeros. With increased size, an explicit setting of the null character is not needed.
OP's code undefined behavior perhaps "works" as a zero may exist just past ...
6
In this answer I'm skipping over most of the implementation and only talking about the file format itself.
How does your game consume this file? Your format should be designed for efficient reading.
I bet your game doesn't use the data in this format. I bet your game engine parses this format and creates a single array with interleaved vertices, normals and ...
6
I'm not sure if it's a good idea to use such identifiers or if floats and integers can be stored in a better way.
Anyway, I want to get tips and criticism.
Declare, not define
pmf_types.h declares and defines various objects. It should only declare them.
// const uint32_t pmf_min_major = 1;
extern const uint32_t pmf_min_major;
and define them in one .c ...
answered Dec 29 '20 at 19:37
chux - Reinstate Monica
20.3k2 gold badges24 silver badges58 bronze badges
5
Code
I would remove the explicit if(offset+val>fileSize) { checks, as well as even getting fileSize in the first place. Instead, replace it with actual error handling, which you completely skip. This properly handles errors such as a disk issue while reading, and allows for flexibility in cases such as streaming a file over the network when you don't know ...
5
Excellent comment in the header. It's very important to be clear who owns the returned memory and how to release it. It ought to mention that both newline and \0 are considered as end-of-line characters.
Good realloc() usage - we don't leak when allocation fails.
Some small improvements:
#define EXPAND_RATIO 2
Why do we need a preprocessor macro for this?...
5
Code
#pragma GCC optimize("O2")
Normally we just pass that as a compiler flag, so we don't get "unrecognised pragma" warnings from other compilers.
long int c,d,j,N,Q;
These are poor names - they tell us very little about what they are used for.
The number of array elements and the number of queries can't be negative. I suggest ...
5
Here are some things that may help you improve your code.
Choose better variable names
I understand that N and Q are used within the problem description, but c and d are not and are very short and non-descriptive names.
Add comments
Comments do not add to compile time and help you (and others!) keep track of what is happening in the code.
Add error checking
...
5
The code can be structured better, so the code is more self-documenting.
Self-documenting code doesn't need as many comments, which makes it easier on everyone. Comments that aren't there can't be obsolete either.
Take input (height)
Output pyramid
Those are the only lines I want to see in main. The rest should be in functions.
You started out nicely by ...
5
Only problem is the goto. Can it be avoided without using a function?
Candidate goto replacement
for (col = bq[row];;) {
//NEXT_col: // Delete
if (++col == N)
bq[row--] = -1;
else {
for (rd = 1; rd <= row; rd++)
if ((old = bq[row-rd]) == col || old == col+rd || old == col-rd)
...
4
Overall, a good and thoughtful effort.
Bug: Treats character as EOF
When EOF == -1 and code reads a character with the value of 255, the c = getc(fp); likely converts that 255 to -1. Later, the loop incorrectly exits.
signed char c;
c = getc(fp);
if (c == '\n' || c == EOF) return NULL; // Maybe true for wrong reason
Instead use an int c to well ...
4
While your code "works" for you, there are a lot of small mistakes in it. I'm listing them straight from top to bottom.
#include "stdio.h"
Since stdio.h is a header from the standard library, as opposed to a header you define yourself in your project, you should include it using #include <stdio.h> instead.
typedef struct Car {
...
3
In the line
printf("\n<Car: %s, Price: $%d>", car->name, car->price);
the variable car->price is of type unsigned int, but you are using %d to print it, which is intended for signed int. This causes undefined behavior according to §7.21.6.1 ¶9 of the ISO C standard. However, on most platforms, this will probably just cause the number ...
3
Overall this isn't a bad effort. I can't see any problems, aside from clobbering the EOF, mostly it's stylistic choices. So good job 👍.
Comments go above
It's much more common for the comment in a header to be ABOVE the function, rather than below it. This is ok for now as there's only one function, but when you have more it will confuse people.
If you're ...
3
malloc vs. realloc
Rather than allocate, copy, append ...
... consider re-allocate, append.
// char* tempFunctionName = (char*)malloc((functionNameLength+2)*sizeof(char));
// strcpy(tempFunctionName, functionName);
// tempFunctionName[functionNameLength] = c;
// tempFunctionName[functionNameLength+1] = 0;
// free(functionName);
// functionName = ...
3
You are relying on the compiler zeroing local variables. For portability initialize them as needed.
Use the address of either head or next fields (llnode**). Then deletion will become trivial.
Free the deleted node.
head node deletion does not printf. As printf was probably just test code, not so important.
Two loops (instead of ifs) are more readable; first ...
3
In the function del_element_from_last, when removing the first element of the list, you update llist->head. However, when removing the last element of the list, you do not update llist->tail.
If keeping track of the tail of the list is only intended for building the list, and is not intended to be updated by the function del_element_from_last, then you ...
3
Intractability
Think hard about what's happening here - you're asking that the user provide a timedelta or time interval that is divorced from absolute time, e.g. 800 days. However, you do not ask that it be specified as "800 days in 2020" or "800 days in 1974". So:
It is impossible to map a number of days to a number of years, months and ...
3
float x;
fread((void*)(&x), sizeof(x), 1, file);
All pointers are convertible to void*, so there's no need for the explicit cast:
fread(&x, sizeof x, 1, file);
However, there's a bigger issue lurking here. In-memory representation of float can vary between platforms. Even if we limit ourselves to platforms using single-width IEEE-...
3
Minimum edit distance
count_edit_dist() does return a value, yet it is certainly not the minimum possible edit distance as defined. To compute the minimum, a fair amount more code is needed.
Small stuff
unsigned vs. size_t vs. int
For practical uses, makes no difference, yet code would avoid casting by using size_t and making the printf() format strings ...
3
"Utility" libraries
It's time to take the training wheels off, so to speak. You need to stop using cs50.h and replace it with standard calls to the C libraries. The implementation for get_string has a careful, dynamically-allocated buffer algorithm that is really not necessary for most purposes and can be replaced with simpler calls that use a ...
3
Here are some ideas that may help you improve your code.
Understand the purpose of CMake
Many people misunderstand CMake as a "build system" but it is not. It is a tool for creating a build system. Generally speaking, this means that you describe the desired artifacts (executables to be compiled, tests to be compiled and run, documentation to be ...
3
void pr_solution(int *bq, int col) {
void init_board(int bq[], int v) {
These could be declared with static linkage.
int main() {
Prefer int main(void) to make this declaration a prototype (takes no arguments, rather than unspecified arguments).
int row, col, rd, old;
All of these could have smaller scope. E.g.
for (int row = 0; row >= 0;) {
It's ...
3
First, the bug
Consider this: what happens if you have a string that is shorter than maxlen as your input?
Assume that maxlen=78, and the input string s is "foo bar". You have strlen(s) == 7. So then pos = 0 at first matches (pos < maxlen), so the while loop starts, the for loop runs, and s[pos + i] is s[0 + maxlen] is s[0 + 78] which is ...
2
Type-size switch. Is this a reasonable techique hint to the compiler to use registers for optimisation rather than slow generic memcpy for small types.
Why do you think that memcpy() has not already implemented this optimization itself? Do you have timing data to suggest this optimization works and improves performance?
Does the above violate the standard? ...
Only top voted, non community-wiki answers of a minimum length are eligible
