some small issues picked up by linter #4

New Issue

asdf · 2020-01-09T02:58:49Z

asdf commented

2020-01-09 02:58:49 +00:00

I was interested in the memory leak but ended up seeing these and thought you might like to know about them (but if not then no worries!):

hermes.c|694 warning| %d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'. 
hermes.c|733 warning| %d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'.
hermes.c|774 warning| %d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'.
hermes.c|776 warning| %d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'.
hermes.c|818 warning| %d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'.
hermes.c|820 warning| %d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'.
hermes.c|837 warning| %d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'.

Colour values from config.h are unsigned int, but are printed using %d in the format string which is for signed int. For example:

static const unsigned int EDITOR_BG_COLOR    = 235;
...
int bglen = sprintf(bgcolor, "33[48;5;%dm", EDITOR_BG_COLOR);
                                         ^

I don't know enough about this, but changing them to %u works. There are other similar statements that use signed values and remain as %d, not sure if this is incongruous.

hermes.c|941 warning| The scope of the variable 'swap' can be reduced.

I guess you could initialise the variable inside the if block. This reminds me that Go has if statement; condition {} which is cool.

hermes.c|1042 error| Common realloc mistake: 'buf' nulled but not freed upon failure

The return value of realloc has to be checked, as it could return null if there is an error.

I was interested in the memory leak but ended up seeing these and thought you might like to know about them (but if not then no worries!): ``` hermes.c|694 warning| %d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'. hermes.c|733 warning| %d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'. hermes.c|774 warning| %d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'. hermes.c|776 warning| %d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'. hermes.c|818 warning| %d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'. hermes.c|820 warning| %d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'. hermes.c|837 warning| %d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'. ``` Colour values from config.h are unsigned int, but are printed using %d in the format string which is for signed int. For example: ``` static const unsigned int EDITOR_BG_COLOR = 235; ... int bglen = sprintf(bgcolor, "�33[48;5;%dm", EDITOR_BG_COLOR); ^ ``` I don't know enough about this, but changing them to %u works. There are other similar statements that use signed values and remain as %d, not sure if this is incongruous. ``` hermes.c|941 warning| The scope of the variable 'swap' can be reduced. ``` I guess you could initialise the variable inside the if block. This reminds me that Go has `if statement; condition {}` which is cool. ``` hermes.c|1042 error| Common realloc mistake: 'buf' nulled but not freed upon failure ``` The return value of realloc has to be checked, as it could return null if there is an error.

👍 1

sloum commented

2020-01-09 03:08:44 +00:00

This is awesome! Thank you so much! I will attend to all of these :)

sloum added the

bug

label 2020-01-09 03:08:49 +00:00

sloum commented

2020-01-09 03:23:12 +00:00

I have updated the unsigned ints to ints (both in config.h and in the function return value for editorSyntaxToColor()). I also moved swap into a smaller scope. I had been using it differently in an earlier edit of that function.

The last one is a bit confusing to me as I am not wanting to free the memory at that particular place in the function. The other if branches do handle freeing it when it is nulled. I may be misunderstanding the linter warning though.

What are you using to generate these? I am using pretty strict messaging during compile, but definitely did not catch these...

I have updated the unsigned ints to ints (both in `config.h` and in the function return value for `editorSyntaxToColor()`). I also moved swap into a smaller scope. I had been using it differently in an earlier edit of that function. The last one is a bit confusing to me as I am _not_ wanting to free the memory at that particular place in the function. The other if branches do handle freeing it when it is nulled. I may be misunderstanding the linter warning though. What are you using to generate these? I am using pretty strict messaging during compile, but definitely did not catch these...

asdf commented

2020-01-09 04:16:10 +00:00

This link explains it better than I could, but I'll try anyway lol: if realloc() fails, it returns null and buf is assigned null. free(buf) will not have any effect, because it's trying to free memory at null location.

The linter I used is cppcheck. It just happened to be the one in the package repo I tried out first.

[This link](https://stackoverflow.com/questions/27589846/dynamic-arrays-using-realloc-without-memory-leaks#27589881) explains it better than I could, but I'll try anyway lol: if `realloc()` fails, it returns null and buf is assigned null. `free(buf)` will not have any effect, because it's trying to free memory at null location. The linter I used is [cppcheck](https://en.wikipedia.org/wiki/Cppcheck). It just happened to be the one in the package repo I tried out first.

sloum commented

2020-01-09 05:20:18 +00:00

Interesting. It makes sense. It is odd that the linter only lit up for one instance. The whole program uses realloc without first assigning it to an intermediary and checking for null. I wonder if it matters what C version you are using? This uses c99... but I'm not sure if that is relevant or not.

I have gotten warning from C++ linters but have generally ignored them as gcc doesnt give me any beef with it for c99, so it is also a possible difference there... though I dont know enough about the C/C++ ecosystem/tooling to know.

I can definitely make that adjustment, and if that is what is happening it sounds like it would be a good idea to do so. I just need to figure if I need to do it everywhere, or if for some reason the issue is specific to the mentioned line.

Interesting. It makes sense. It is odd that the linter only lit up for one instance. The whole program uses `realloc` without first assigning it to an intermediary and checking for null. I wonder if it matters what C version you are using? This uses c99... but I'm not sure if that is relevant or not. I have gotten warning from C++ linters but have generally ignored them as gcc doesnt give me any beef with it for c99, so it is also a possible difference there... though I dont know enough about the C/C++ ecosystem/tooling to know. I can definitely make that adjustment, and if that is what is happening it sounds like it would be a good idea to do so. I just need to figure if I need to do it everywhere, or if for some reason the issue is specific to the mentioned line.

asdf commented

2020-01-10 02:06:05 +00:00

That's a really good point, I'm not sure why this is the one it picks up and not the other instances. That does not really make sense. Plus, it was just the first linter I checked for testing purposes.

The part I missed in my explanation is that if realloc fails, the original pointer to buf is lost, and that memory leaks. But you might have larger problems if this occurs.

realloc and malloc both return null on failure. I think generally these should be checked, but I'm not too sure about this and tend to get bogged down at points like these. Either way, keep on truckin'

That's a really good point, I'm not sure why this is the one it picks up and not the other instances. That does not really make sense. Plus, it was just the first linter I checked for testing purposes. The part I missed in my explanation is that if realloc fails, the original pointer to buf is lost, and that memory leaks. But you might have larger problems if this occurs. realloc and malloc both return null on failure. I think generally these should be checked, but I'm not too sure about this and tend to get bogged down at points like these. Either way, keep on truckin'

Sign in to join this conversation.