Reduced compiler warnings #196
Conversation
|
How does this improve security exactly? I don't know the policy behind this, but I don't think you should be incrementing version numbers or including yourself as a key contributor without consultation. You're also breaking functionality for some reason. Personally I question whether Regardless, reducing compiler warnings seems sensible, but note that this project is largely unmaintained, so this PR may not get merged for quite some time (if ever). |
|
Hey, thanks for the feedback! You’re absolutely right to question the “security” claim—I actually made these changes a few weeks ago, and this morning when writing this PR I mistakenly recalled working on a buffer-related issue. Looking through the changes, that was incorrect, and I’ll update the title to reflect what this PR actually does (compiler warning reductions). I’ll also remove the version bump and contributor changes—totally fair. I'm also happy to remove the -Werror and -ffast-math compiler flags if you think it would be problematic? Appreciate the review! |
TheJostler
changed the title
|
I know this will be sortof annoying, but could you undo the permissions changes on the files. This is an unwanted change |
To be brutally honest, I think this PR just screams 'sloppy' at best. Your mention of recently "Looking through the changes" clearly shows it wasn't reviewed before submission, which was what I had initially thought. If the goal is to reduce compiler warnings, the PR should only contain changes related to such. If it's more than just that, you should point that out. Your changes to src/commandline.cpp are still obviously wrong. Inserting |
There was a problem hiding this comment.
this should have been a separate commit since this update is generated
| { | ||
| case 'g': | ||
| redundancysize = redundancysize * 1024; | ||
| break; |
There was a problem hiding this comment.
This must deliberatly fallthrough so the reduncancysize is multiplied more with the given modifier
| break; | ||
| case 'm': | ||
| redundancysize = redundancysize * 1024; | ||
| break; |
There was a problem hiding this comment.
This must deliberatly fallthrough so the reduncancysize is multiplied more with the given modifier (same as above )
|
|
||
| template <> bool ReedSolomon<Galois8>::SetInput(const vector<bool> &present, std::ostream &sout, std::ostream &serr) | ||
| { | ||
| (void) sout; |
There was a problem hiding this comment.
Could you elaborate why this is added and potentially separate these changes in a separate commit?
There was a problem hiding this comment.
Could you elaborate here why this change is needed and potentially separate it in a commit with the explanation?
There was a problem hiding this comment.
The removal of blocksize (unused I guess) should also be a separate commit with some explanation
also referring to
- src/par2repairer.cpp
- src/verificationhashtable.h
|
|
||
| // Allocate the right hand matrix only if we are recovering | ||
| // Use value initialization to initialize each element in the matrix to zero (or its default state) | ||
| for (size_t i = 0; i < outcount * incount; ++i) { |
There was a problem hiding this comment.
I think we can just remove the memset in this case and there is no need to pre-allocate anything since the next part will fill the matrix anyway, same for the rightmatrix
| // read little-endian value | ||
| utype v = 0; | ||
| for (int byte_index = 0; byte_index < sizeof(utype); byte_index++) { | ||
| for (long unsigned int byte_index = 0; byte_index < sizeof(utype); byte_index++) { |
There was a problem hiding this comment.
long unsigned int? Who would ever write this? Was this code change generated by an AI?
There was a problem hiding this comment.
No it was not, this change was made to reduce compiler warnings as the name of the PR suggests
There was a problem hiding this comment.
Check the C++ standard (any version). There are no long unsigned int. It should have been unsigned long int.
There was a problem hiding this comment.
Given that byte_index is compared with a sizeof() it should be of type size_t. Unless C++ differs from C here.
There was a problem hiding this comment.
You're right Rhialto, it should be size_t.
|
Kindly split this pull request in multiple ones. At least, the file permission changes should be a pull request distinct from the code changes, so that would be 2 PR instead of 1. You may further split the code changes if some of them are tricky or unsafe or subject to controversy, thanks. |
5 participants
I've configured make to set the
-Werror -Wextraflags and resolved any discrepancies within the codebase in order to reduce the compiler warnings.