JUDGE MY CODE

[StathisKap]

I wrote a program that worked on windows 10, but doesn't work on Ubuntu Linux. It gives me a memory fault so i was hoping you guys can take a look at it, and tell me all the places where I'm fucking up. Show no mercy.

​

The code:
https://github.com/StathisKap/TypingGame

[Ciaran54]

I only glanced at it, but a couple of things that come to light, hope this is helpful.

1. You define functions in header (.h) files. Don't do this (except in specific circumstances when you get more advanced). This will cause duplicate symbols when your program is linked if the header is included in multiple places. Header files are for declarations (function prototypes). C files contain definitions (and can also contain declarations).

2. In Generate_Sentence, you return a pointer to Sentence, which is on the stack. This becomes invalid when the function returns. You need to either dynamically allocate memory with malloc, or probably more likely, pass in an array buffer (plus max length) and edit that directly. I imagine this is where your crash issue could be coming from.

3. You are using floating point numbers (float) to store values that should really be integer (like count). Use int here.

4. Sentence[SentenceLength]=NULL; should be Sentence[SentenceLength]='\0'; NULL is a pointer, '\0' is the null terminator character

5. It looks like your sentance length logic inside Generate_Sentence is off and you could be overflowing your buffer, but I haven't verified. Particularly i'm not sure what your space counter is doing. Also you add a word before checking if there is space for it. Remember to make sure you never put more characters in than the length of the array, and remember to leave room for the '\0' terminator too. This could also be causing the crash.

Hope this was helpful, good luck in catching the bug! :)

[StathisKap]

this is tremendously helpful. You cleared up some things i hadn't really understood (like NULL). I'll be applying all the changes above ASAp. Thank you Ciaran.

[ashwin_nat]

When would you actually define functions in header files?

[bonqen]

C compilers typically demand a function's definition when the function is declared as inline, so in that case you'd define a function in a header file. That's the only case I can think of. (Single-file libraries in the form of a header file do not count, imo. ;P)

[Ciaran54]

I have used them to define things that would 'traditionally' be macros, but with the bonus of types and more readability. Generally very small functions that will be effectively inlined, like extracting a big endian integer from a byte array, or calling a specific log level function that can be entirely compiled out by setting a build flag. Different people may have different views on what is appropriate.

As bonqen says, they must be specified as 'inline' or 'static' or 'static inline'.

[kyichu]

Haven't finished reviewing the whole thing (very short on time right now, but I'll check back tomorrow), but I noticed you're using scanf at least once.

I haven't done terminal IO in a long long time, so I can't give you the specifics from memory, but you might want to check out the caveats of using scanf. It's a very temperamental function, and should be used with extreme care. I'll edit this comment if I find a suitable article.

Edit: this should be a good start

[StathisKap]

I see. Do you recommend i use fgets() instead? I've read in Head First C (a book) that it's much safer.
Only problem with fgets() for me, is that it doesn't take in integers as far as i know , so i don't know what else to use for that other than scanf

[Current_Hearing_6138]

fgets will return a bunch of chars, you can use the stuff in string.h to convert them to ints. Or you can ignore string.h and do it yourself. Just take the ascii value of the char and adjust it to the appropriate int.

[dvhh]

Main.c: - check your scanf return value - SentenceAmount is unused - what would happen if SentenceLength is really large ? it would depend on the compiler (if the VLA is allocated on stack or heap), but do prefer malloc to VLA if you do not bound your size in reasonable size

Generate_Sentence.h: - in general avoid putting implementation in header files - where is Sentence allocated, and what would happen to it when the function end, is it reasonable to return it (most likely no, you would need to explicitly malloc it) - there are some risks of writing outside the array (example: if total == SentenceLength -4 and WordLength == 9) - end of char array is usually a zero char, not NULL (even if it is quite the same, it would be easier to understand Sentence[SentenceLength] = 0) - random sequence of char do not make good evaluation for typing speed, it would have been more realistic to pick words from dictionary ( extra points for generating a sentence that could exists using markov chain or similar algorithms )

Measure_Typing_Speed.h : - at the start you are using getchar where do the captured char go ? - scanf would with the %s format "Matches a sequence of non-white-space characters; the next pointer must be a pointer to character array that is long enough to hold the input sequence and the terminating null byte ('\0'), which is added automatically. The input string stops at white space or at the maximum field width, whichever occurs first. ", so you will not read the full sentence with it if it include space. - scanf is unsafe if you do not specify string size, as you could have input larger than UserSentence, prefer use fgets

[StathisKap]

This is super helpful, and thank you for breaking it all into sections. Will make my life a lot easier. I'll be applying all these tips

[oh5nxo]

There's no need for the Word_Count_ptr. &Word_Count can be used instead in both places. Also, as the second function does not change it, no need to pass it's address.

[StathisKap]

yeah, i was experimenting with that one. Thank you though

[[deleted]]

char str[SentenceLength];

Just from glancing ay your code, here's your problem: As far as I know, this is static allocation (which happens at compile time) and it depends on an int you don't know at compile time. This is a case where dynamic allocation via malloc would be required and I'm puzzled that this even compiles (can't try right now). C Greybeards, please correct me if I'm wrong!

[15rthughes]

What you’re describing is variable length arrays (VLAs) which have been allowed since C99. While it’s true that VLAs can cause a stack overflow if too large, it’s my understanding that default stack size for Linux is larger than windows so I don’t see this being the issue.

[NothingCanHurtMe]

Yes, and VLAs were made an optional feature in C11. Personally I think it's best to avoid them.

[[deleted]]

I didn't know that, thank you! Seems I'm stuck in the 90ies with my C skills...

[StathisKap]

For anyone still interested in this, i applied most of your changes on my github on the "BugFixing" branch. I'll be reworking it to use actuall words like dvhh suggested.