Fix clang-tidy issues in net classes#7593
Conversation
Unused variables, potential memory leak, wrong strlcpy arguemnt
Memory can be leaked in case of any network error
|
Starting build on |
| Bool_t update = (fOption == "UPDATE") ? kTRUE : kFALSE; | ||
| Bool_t read = (fOption == "READ") ? kTRUE : kFALSE; | ||
| if (!create && !recreate && !update && !read) { | ||
| read = kTRUE; |
There was a problem hiding this comment.
This is one code pattern where I see "some" value in updating the variable even if it is, currently, not used.
If this routine is update at a later date, the developer might assume that the variable 'read' is true is all cases of 'reading mode' even the default one.
On the other hand, not having it at all might be event clearer. i.e. except for the slight extra run-time cost, I think the fillowing code has the same net effect:
Bool_t update = (fOption == "UPDATE") ? kTRUE : kFALSE;
if (!create && !recreate && !update) {
fOption = "READ";
}
with a spurrious re-assignment in case fOption == "READ" ...
So actually maybe even:
Bool_t update = (fOption == "UPDATE") ? kTRUE : kFALSE;
if (!create && !recreate && !update && (fOption != "READ")) {
fOption = "READ";
}
There was a problem hiding this comment.
I am not sure.
fOption can be accessed via TFile::GetOption() method - user can always check that was original option he/she provide.
None of other TFile-based classes, including TFile itself, change fOption in such case.
Either we do it consistently in all places, or do not change it here.
There was a problem hiding this comment.
None of other TFile-based classes, including TFile itself, change fOption in such case.
Sorry, I am wrong. I will update PR
|
Build failed on ROOT-fedora30/cxx14. Failing tests: |
"READ" is default mode and assigned if none of other modes are recognized. No need for extra `read` flag.
|
Starting build on |
|
Build failed on windows10/cxx14. Errors:
|
3 participants
Fixes #7528
Civetweb not touched, need to be updated soon anyway