New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: supress GCC13 warnings #24434
fix: supress GCC13 warnings #24434
Conversation
@@ -149,7 +149,7 @@ void getPoolingKernelParams(const LayerParams ¶ms, std::vector<size_t>& kern | |||
std::vector<size_t>& strides, cv::String &padMode) | |||
{ | |||
bool is_global = params.get<bool>("global_pooling", false); | |||
globalPooling.resize(3); | |||
globalPooling.reserve(3); | |||
globalPooling[0] = params.get<bool>("global_pooling_d", is_global); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be .resize()
.reserve()
is wrong (during reallocation container will lost stored data)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your review!
I believe that stored data in globalPooling
should not be cared.
- After
globalPooling.resize(3)
, data inglobalPooling[0..2]
are kept. IfglobalPooling
has more data, they will be lost. - Data in
globalPooling[0..2]
are overwrited following statements. So they will be lost.
opencv/modules/dnn/src/layers/layers_common.cpp
Lines 147 to 156 in c2f909f
void getPoolingKernelParams(const LayerParams ¶ms, std::vector<size_t>& kernel, std::vector<bool>& globalPooling, | |
std::vector<size_t>& pads_begin, std::vector<size_t>& pads_end, | |
std::vector<size_t>& strides, cv::String &padMode) | |
{ | |
bool is_global = params.get<bool>("global_pooling", false); | |
globalPooling.resize(3); | |
globalPooling[0] = params.get<bool>("global_pooling_d", is_global); | |
globalPooling[1] = params.get<bool>("global_pooling_h", is_global); | |
globalPooling[2] = params.get<bool>("global_pooling_w", is_global); | |
(if it is correct) I'm sorry that pull request code has a bug.
globalPooling.clear()
is needed to reset verctor elements before globalPooling.reserve(3)
.
globalPooling.size()
will be update when set globalPooling[0...2] = ...
.
I think it will be the same condition as it was before this fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this may be a gcc misjudgment. But vector<bool>
is special complex case to handle by each bit. It is possible that an incorrect memory copy range bug may occur really when vector::resize() is called.
So I believe that avoiding to call resize()
is better than suppessing warning.
kmtr@kmtr-None:~/work/build4-main$ vi ../opencv/modules/dnn/src/layers/layers_common.cpp
kmtr@kmtr-None:~/work/build4-main$ ninja
[1/19] Building CXX object modules/dnn/CMakeFiles/opencv_dnn.dir/src/layers/layers_common.cpp.o
In file included from /usr/include/c++/13/array:43,
from /home/kmtr/work/opencv/modules/core/include/opencv2/core/cvdef.h:794,
from /home/kmtr/work/opencv/modules/core/include/opencv2/core.hpp:52,
from /home/kmtr/work/opencv/modules/dnn/src/layers/../precomp.hpp:49,
from /home/kmtr/work/opencv/modules/dnn/src/layers/layers_common.cpp:43:
In static member function ‘static _Up* std::__copy_move<_IsMove, true, std::random_access_iterator_tag>::__copy_m(_Tp*, _Tp*, _Up*) [with _Tp = long unsigned int; _Up = long unsigned int; bool _IsMove = false]’,
inlined from ‘_OI std::__copy_move_a2(_II, _II, _OI) [with bool _IsMove = false; _II = long unsigned int*; _OI = long unsigned int*]’ at /usr/include/c++/13/bits/stl_algobase.h:506:30,
inlined from ‘_OI std::__copy_move_a1(_II, _II, _OI) [with bool _IsMove = false; _II = long unsigned int*; _OI = long unsigned int*]’ at /usr/include/c++/13/bits/stl_algobase.h:533:42,
inlined from ‘_OI std::__copy_move_a(_II, _II, _OI) [with bool _IsMove = false; _II = long unsigned int*; _OI = long unsigned int*]’ at /usr/include/c++/13/bits/stl_algobase.h:540:31,
inlined from ‘_OI std::copy(_II, _II, _OI) [with _II = long unsigned int*; _OI = long unsigned int*]’ at /usr/include/c++/13/bits/stl_algobase.h:633:7,
inlined from ‘std::vector<bool, _Alloc>::iterator std::vector<bool, _Alloc>::_M_copy_aligned(const_iterator, const_iterator, iterator) [with _Alloc = std::allocator<bool>]’ at /usr/include/c++/13/bits/stl_bvector.h:1306:28,
inlined from ‘void std::vector<bool, _Alloc>::_M_fill_insert(iterator, size_type, bool) [with _Alloc = std::allocator<bool>]’ at /usr/include/c++/13/bits/vector.tcc:879:34,
inlined from ‘std::vector<bool, _Alloc>::iterator std::vector<bool, _Alloc>::insert(const_iterator, size_type, const bool&) [with _Alloc = std::allocator<bool>]’ at /usr/include/c++/13/bits/stl_bvector.h:1206:16,
inlined from ‘void std::vector<bool, _Alloc>::resize(size_type, bool) [with _Alloc = std::allocator<bool>]’ at /usr/include/c++/13/bits/stl_bvector.h:1252:10,
inlined from ‘void cv::dnn::getPoolingKernelParams(const dnn4_v20230620::LayerParams&, std::vector<long unsigned int>&, std::vector<bool>&, std::vector<long unsigned int>&, std::vector<long unsigned int>&, std::vector<long unsigned int>&, cv::String&)’ at /home/kmtr/work/opencv/modules/dnn/src/layers/layers_common.cpp:152:25:
/usr/include/c++/13/bits/stl_algobase.h:437:30: warning: ‘void* __builtin_memmove(void*, const void*, long unsigned int)’ forming offset 8 is out of the bounds [0, 8] [-Warray-bounds=]
437 | __builtin_memmove(__result, __first, sizeof(_Tp) * _Num);
| ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[19/19] Linking CXX executable bin/opencv_test_gapi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this will work?
globalPooling.assign({
params.get<bool>("global_pooling_d", is_global),
params.get<bool>("global_pooling_h", is_global),
params.get<bool>("global_pooling_w", is_global),
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for your comments, I agree with you !
modules/stitching/src/camera.cpp
Outdated
@@ -66,7 +66,7 @@ Mat CameraParams::K() const | |||
Mat_<double> k = Mat::eye(3, 3, CV_64F); | |||
k(0,0) = focal; k(0,2) = ppx; | |||
k(1,1) = focal * aspect; k(1,2) = ppy; | |||
return std::move(k); | |||
return k; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe so?
return k; | |
return Mat(k); |
Or the whole function should be rewritten, so that local variable being returned would match returned type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it works well, thank you ! GCC13 suggested to remove std::move()
because it is redundant code. However clang on macOS/xcode suggests to keep std:move()
because type conversion. I think using Mat()
is simple and the best instead of supressing warning.
} | ||
|
||
flow(i, j) = u; | ||
} | ||
} | ||
file.close(); | ||
return std::move(flow); | ||
return flow; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as previous comment.
return flow; | |
return Mat(flow); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the above, I also modified the code to use Mat()
. thank you very much !
* fix: supress GCC13 warnings * fix for review and compile-warning on MacOS
* fix: supress GCC13 warnings * fix for review and compile-warning on MacOS
For ubuntu 23.10, supress GCC13 warnings.
OpenCV version: 4.x
Operating System / Platform: Ubuntu 23.10
Compiler & compiler version: GCC 13.2.0
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.