Some source code issues
Moderator: GZDoom Developers
Forum rules
Please don't bump threads here if you have a problem - it will often be forgotten about if you do. Instead, make a new thread here.
Please don't bump threads here if you have a problem - it will often be forgotten about if you do. Instead, make a new thread here.
-
- Posts: 19
- Joined: Thu Aug 10, 2017 8:09 pm
Some source code issues
1. https://github.com/coelckers/gzdoom/blo ... s.cpp#L941
Uninitialized variable fbestdist
2. https://github.com/coelckers/gzdoom/blo ... ors.h#L651
Non-void function should return a value
3. https://github.com/coelckers/gzdoom/blo ... .cpp#L1226
Copypast ? box.right > 0 && box.right > 0
4. https://github.com/coelckers/gzdoom/blo ... w.cpp#L194
if (next_random == FUZZ_RANDOM_X_SIZE) always false
5. https://github.com/coelckers/gzdoom/blo ... e.cpp#L589
Expression always false
6. https://github.com/coelckers/gzdoom/blo ... e.cpp#L113
Expression always true
7. https://github.com/coelckers/gzdoom/blo ... .cpp#L9028
Expression always false
8.https://github.com/coelckers/gzdoom/blo ... T.cpp#L526
if (size >= 623), else if (size >= 68), else if (size >= 39)
Expression always false
9. https://github.com/coelckers/gzdoom/blo ... .cpp#L2836
Expression always true
10. https://github.com/coelckers/gzdoom/blo ... s.cpp#L198 and
https://github.com/coelckers/gzdoom/blo ... s.cpp#L208
Expressions always true
11. https://github.com/coelckers/gzdoom/blo ... .cpp#L5515 and
https://github.com/coelckers/gzdoom/blo ... .cpp#L6314
Expressions always true
12.https://github.com/coelckers/gzdoom/blo ... .cpp#L6348
Some part of expression always true: mobj
13.https://github.com/coelckers/gzdoom/blo ... .cpp#L6527
Some part of expression always true: !checker
Uninitialized variable fbestdist
2. https://github.com/coelckers/gzdoom/blo ... ors.h#L651
Non-void function should return a value
3. https://github.com/coelckers/gzdoom/blo ... .cpp#L1226
Copypast ? box.right > 0 && box.right > 0
4. https://github.com/coelckers/gzdoom/blo ... w.cpp#L194
if (next_random == FUZZ_RANDOM_X_SIZE) always false
5. https://github.com/coelckers/gzdoom/blo ... e.cpp#L589
Expression always false
6. https://github.com/coelckers/gzdoom/blo ... e.cpp#L113
Expression always true
7. https://github.com/coelckers/gzdoom/blo ... .cpp#L9028
Expression always false
8.https://github.com/coelckers/gzdoom/blo ... T.cpp#L526
if (size >= 623), else if (size >= 68), else if (size >= 39)
Expression always false
9. https://github.com/coelckers/gzdoom/blo ... .cpp#L2836
Expression always true
10. https://github.com/coelckers/gzdoom/blo ... s.cpp#L198 and
https://github.com/coelckers/gzdoom/blo ... s.cpp#L208
Expressions always true
11. https://github.com/coelckers/gzdoom/blo ... .cpp#L5515 and
https://github.com/coelckers/gzdoom/blo ... .cpp#L6314
Expressions always true
12.https://github.com/coelckers/gzdoom/blo ... .cpp#L6348
Some part of expression always true: mobj
13.https://github.com/coelckers/gzdoom/blo ... .cpp#L6527
Some part of expression always true: !checker
-
- Posts: 1774
- Joined: Sat Oct 17, 2009 9:40 am
Re: Some source code issues
Did you use a tool for this?
-
- Posts: 19
- Joined: Thu Aug 10, 2017 8:09 pm
Re: Some source code issues
I can check with PVS-studio and send Log file
This ten errors i found randomly tring adding IMGUI
This ten errors i found randomly tring adding IMGUI
Re: Some source code issues
Why are you using different commit numbers for each and every line here? Most of these are more nitpicks than they are real problems. What is the purpose of this report?
Re: Some source code issues
Those are commits when corresponding files were changed the last time.
And I wouldn’t say that it’s just a nitpicking. Most of them seem to be logic errors.
And I wouldn’t say that it’s just a nitpicking. Most of them seem to be logic errors.
Re: Some source code issues
Fixed a few
I am not touching the ones I either cannot follow, or seem to be safety bars, or seem to be compatibility hacks. For any of the latter two, I think it should be Graf, exclusively, who deals with those.
For example, the PCX thing, we could simplify the statement quite a bit by removing the check, but at the same time it may be there for a reason despite the check always being false.
I am not touching the ones I either cannot follow, or seem to be safety bars, or seem to be compatibility hacks. For any of the latter two, I think it should be Graf, exclusively, who deals with those.
For example, the PCX thing, we could simplify the statement quite a bit by removing the check, but at the same time it may be there for a reason despite the check always being false.
- Graf Zahl
- Lead GZDoom+Raze Developer
- Posts: 49073
- Joined: Sat Jul 19, 2003 10:19 am
- Location: Germany
Re: Some source code issues
Looking at those 3, I think you did one wrong and one not optimally.
If you want to set a double to the maximum, use DBL_MAX, not some hard coded constant.
And the third one should check height instead of the second 'width'.
The rest: Most should not be changed, unless the message indicates a missing pointer check outside the always true/false condition.
If you want to set a double to the maximum, use DBL_MAX, not some hard coded constant.
And the third one should check height instead of the second 'width'.
The rest: Most should not be changed, unless the message indicates a missing pointer check outside the always true/false condition.
Re: Some source code issues
The one in r_draw.cpp should be changed to the following:
static int next_random = 0;
I must have been blind when coding the upscaled fuzz demon version for not noticing this bug.
static int next_random = 0;
I must have been blind when coding the upscaled fuzz demon version for not noticing this bug.
Re: Some source code issues
FixedGraf Zahl wrote:Looking at those 3, I think you did one wrong and one not optimally.
If you want to set a double to the maximum, use DBL_MAX, not some hard coded constant.
And the third one should check height instead of the second 'width'.
The rest: Most should not be changed, unless the message indicates a missing pointer check outside the always true/false condition.
Re: Some source code issues
DonedpJudas wrote:The one in r_draw.cpp should be changed to the following:
static int next_random = 0;
I must have been blind when coding the upscaled fuzz demon version for not noticing this bug.
-
- Posts: 19
- Joined: Thu Aug 10, 2017 8:09 pm
Re: Some source code issues
Good job guys!
I checked Gzdoom project in PVS-studio, here results, maybe it's useful info
https://www.dropbox.com/s/g8d3dgjiyznnn ... .html?dl=0
I checked Gzdoom project in PVS-studio, here results, maybe it's useful info
https://www.dropbox.com/s/g8d3dgjiyznnn ... .html?dl=0
Re: Some source code issues
Thanks for trying to help.
Unfortunately we'd rather not have people spamming us with output from random tools unless it serves a very specific purpose. I quote from the code submission guidelines:
Unfortunately we'd rather not have people spamming us with output from random tools unless it serves a very specific purpose. I quote from the code submission guidelines:
Code sanitizer/static analyzer is not an excuse to contribute
If you don't know what you're doing, do not use these tools. We typically find that people who use static analyzers as an excuse to contribute apply the wrong fix. Similarly reporting the errors to us isn't especially helpful as it can become a cat and mouse game. We're not saying they're bad tools, but use them in your own projects first.
Re: Some source code issues
I realize you are trying to be helpful, but running a code analyzer is anything but. We have access to such tools, too, and will use them as needed. No one asked you for your PVS-studio report, and if they wanted it they would have asked.
For the most part I still feel like a lot of this stuff here is nit-picky - some of the code is the way it is for a reason, and while it is good to be aware that it is that way, it does not do us a lot of good with a project that has so many contributors. Believe it or not, some redundancy that you pointed out is actually needed.
In the future I would ask you bear this in mind - if you are going to run PVS-studio over GZDoom then you need to keep 3 things in mind:
a) First, you must be willing to change the code yourself.
b) Second, you must understand the reason why the code is the way it is. If changing it defeats the purpose, then we will not accept the change.
c) Lastly, you must also ensure that any changes made would not ultimately break anything. When running PVS-studio or something else that big which makes such a massive report, the room for error is just too significant in order to solve all the problems. It would take a combined effort of the 4 of us to fix all such issues, and - to put it lightly - "ain't nobody got time for that".
We welcome assistance, but any sort of "here's 2000 problems, gotta go bye!" is not what we're looking for. If you think you can whittle these problems down, you're welcome to make a pull request. But otherwise, I feel this report has been addressed to a satisfactory degree, and I am going to close it.
For the most part I still feel like a lot of this stuff here is nit-picky - some of the code is the way it is for a reason, and while it is good to be aware that it is that way, it does not do us a lot of good with a project that has so many contributors. Believe it or not, some redundancy that you pointed out is actually needed.
In the future I would ask you bear this in mind - if you are going to run PVS-studio over GZDoom then you need to keep 3 things in mind:
a) First, you must be willing to change the code yourself.
b) Second, you must understand the reason why the code is the way it is. If changing it defeats the purpose, then we will not accept the change.
c) Lastly, you must also ensure that any changes made would not ultimately break anything. When running PVS-studio or something else that big which makes such a massive report, the room for error is just too significant in order to solve all the problems. It would take a combined effort of the 4 of us to fix all such issues, and - to put it lightly - "ain't nobody got time for that".
We welcome assistance, but any sort of "here's 2000 problems, gotta go bye!" is not what we're looking for. If you think you can whittle these problems down, you're welcome to make a pull request. But otherwise, I feel this report has been addressed to a satisfactory degree, and I am going to close it.