fix opencv/opencv#20544 nodiscard for msvc/gcc by diablodale · Pull Request #20622 · opencv/opencv

diablodale · 2021-08-27T23:18:27Z

fix #20544 with nodiscard for msvc/gcc and applying it to the most common static factories in the api.
PR is set on branch 3.4. Should be able to apply also on 4.x with some minor work.

⚠ 4.x might have additional static factories which need this improved CV_NODISCARD_STD.
I need to know if you will do the additonal work in the up-merge.
Or if I need to do the work with a 4.x specific commit after you up-merge.
Please tell me. 😀

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

I agree to contribute to the project under Apache 2 License.
To the best of my knowledge, the proposed patch is not based on a code under GPL or other license that is incompatible with OpenCV
The PR is proposed to proper branch
There is reference to original bug report and related work
The feature is well documented and sample code can be built with the project CMake

force_builders=Custom
build_image:Custom=ubuntu:20.04
buildworker:Custom=linux-1

alalek

need to do the work with a 4.x specific commit after you up-merge.

I believe separate additional commit after "merge 3.4" should be better.

alalek · 2021-08-27T23:33:54Z

modules/core/include/opencv2/core/cvdef.h

+* encourages the compiler to issue a warning if the return value is discarded            *
+\****************************************************************************************/
+#ifndef CV_NODISCARD_STD
+#  if defined(__has_cpp_attribute) && __has_cpp_attribute(nodiscard)


__has_cpp_attribute(nodiscard) should be checked separately (through own #if statement)

I disagree. The preprocessor follows strict C logic short-circuiting. Therefore if it doesnt support the _has_cpp_attribute quasi-macro, then the part after the && doesn't run.
In addition it is important that both are true. Otherwise I have to write duplicated code so that I correct handle the case of a compiler supporting _has_cpp_attribute but does not support the nodiscard attribute...in that case, it needs to continue on to the other #elif clauses and not exit the entire if chain.

What is your thinking to separate them?

There are several failed build configurations on CI.

Enjoy.

Thanks for repro. I can fix that. LLVM had a similar problem https://reviews.llvm.org/D57848
These outdated compilers with OSs past end-of-life are troublesome. ✂☠⏲

I'll check the CI results when the CI system is out of maintenance. I checked the PR on the 5 OSs that I listed. Ubuntu 14 w/ gcc 4.x is two years beyond end-of-life so I did not check with it.

Build logs from CI should be available now.

diablodale · 2021-08-28T13:19:55Z

Still no repro I can find that shows the old gcc-only __attribute method works before gcc 7.x on actual opencv code, built with actual opencv build infrastructure.

I do not recommend we investigate too much further into the gcc-only method. It is limited both in platforms and in features. Instead, to focus on the standard [[]] attribute method which is supported in relevant platforms gcc, clang, and msvc.

No repro

mat.hpp has the old macro on the declaration Mat clone() const CV_NODISCARD;

In matrix_transform.cpp in cv::flip add

void flip( InputArray _src, OutputArray _dst, int flip_mode )
{
    CV_INSTRUMENT_REGION();

    // this should issue compiler warning
    Mat cataMat(10, 10, CV_8UC1);
    cataMat.clone();

    CV_Assert( _src.dims() <= 2 );
...

Ubuntu 14 with gcc 4.8.4

[build] [44/103  42% :: 20.493] Building CXX object modules/core/CMakeFiles/opencv_core.dir/src/matrix_operations.cpp.o
[build] [45/103  43% :: 20.720] Building CXX object modules/core/CMakeFiles/opencv_core.dir/src/matrix_transform.cpp.o
[build] [46/103  44% :: 20.964] Building CXX object modules/core/CMakeFiles/opencv_core.dir/src/matrix_sparse.cpp.o

Ubuntu 16 with gcc 5.4.0

[build] [45/103  43% :: 12.555] Building CXX object modules/core/CMakeFiles/opencv_core.dir/src/matrix_sparse.cpp.o
[build] [46/103  44% :: 12.899] Building CXX object modules/core/CMakeFiles/opencv_core.dir/src/matrix_transform.cpp.o
[build] [47/103  45% :: 12.934] Building CXX object modules/core/CMakeFiles/opencv_core.dir/src/matrix_wrap.cpp.o

Debian stretch with gcc 6.3.0

[build] [55/103  40% :: 20.798] Building CXX object modules/core/CMakeFiles/opencv_core.dir/src/matrix_iterator.cpp.o
[build] [56/103  41% :: 21.641] Building CXX object modules/core/CMakeFiles/opencv_core.dir/src/matrix_transform.cpp.o
[build] [57/103  42% :: 21.778] Building CXX object modules/core/CMakeFiles/opencv_core.dir/src/matrix_sparse.cpp.o

Ubuntu 18 with gcc 7.5.0

[build] [55/104  40% :: 21.153] Building CXX object modules/core/CMakeFiles/opencv_core.dir/src/matrix_expressions.cpp.o
[build] [56/104  41% :: 24.333] Building CXX object modules/core/CMakeFiles/opencv_core.dir/src/matrix_transform.cpp.o
[build] ../modules/core/src/matrix_transform.cpp: In function ‘void cv::flip(cv::InputArray, cv::OutputArray, int)’:
[build] ../modules/core/src/matrix_transform.cpp:712:20: warning: ignoring return value of ‘cv::Mat cv::Mat::clone() const’, declared with attribute warn_unused_result [-Wunused-result]
[build]      cataMat.clone();
[build]                     ^
[build] In file included from ../modules/core/include/opencv2/core.hpp:58:0,
[build]                  from ../modules/core/include/opencv2/core/utility.hpp:56,
[build]                  from ../modules/core/src/precomp.hpp:49,
[build]                  from ../modules/core/src/matrix_transform.cpp:5:
[build] ../modules/core/include/opencv2/core/mat.hpp:1214:9: note: declared here
[build]      Mat clone() const CV_NODISCARD;
[build]          ^~~~~
[build] [57/104  42% :: 24.481] Building CXX object modules/core/CMakeFiles/opencv_core.dir/src/matrix_sparse.cpp.o

diablodale · 2021-08-28T15:15:59Z

Also, where/how in OpenCVCompilerOptions.cmake in the MSVC section do I add a needed warning?
I want to add /w15240
https://docs.microsoft.com/en-us/cpp/preprocessor/compiler-warnings-that-are-off-by-default?view=msvc-160

It is off by default, yet very important for correct use of attributes [[]] with MSVC. Any new code added to OpenCV is an opportunity for attributes. Yet, if the attribute is places in a syntatically wrong location, MSVC silently ignores it. For example, while coding/testing this PR I was putting the attribute after static but it needs to be before static. MSVC was silently ignoring it which led to no warning being issued. Naturally, the entire goal of nodiscard is to be warned, so it is also important that the attribute itself is places in the correct location. This warning does that.

alalek · 2021-08-28T16:28:23Z

Here (under MSVC section after OPENCV_EXTRA_ flags): https://github.com/opencv/opencv/blob/4.5.3/cmake/OpenCVCompilerOptions.cmake#L386
Code should look like: add_extra_compiler_option("/w15240")

- includes workaround for preprocessor non-compliance - enable attribute syntax checking in msvc

alalek

Looks good to me! Thank you 👍

xref: #11990

alalek reviewed Aug 27, 2021

View reviewed changes

fix #20544 nodiscard for msvc/gcc

3995dea

- includes workaround for preprocessor non-compliance - enable attribute syntax checking in msvc

alalek approved these changes Aug 30, 2021

View reviewed changes

opencv-pushbot merged commit 7eaadf6 into opencv:3.4 Aug 30, 2021

alalek mentioned this pull request Sep 1, 2021

(4.x) Merge 3.4 #20642

Merged

alalek mentioned this pull request Oct 15, 2021

(5.x) Merge 4.x #20886

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix opencv/opencv#20544 nodiscard for msvc/gcc#20622

fix opencv/opencv#20544 nodiscard for msvc/gcc#20622
opencv-pushbot merged 1 commit intoopencv:3.4from
diablodale:fix20544-nodiscard

diablodale commented Aug 27, 2021 •

edited by alalek

Loading

Uh oh!

alalek left a comment

Uh oh!

alalek Aug 27, 2021

Uh oh!

diablodale Aug 28, 2021

Uh oh!

alalek Aug 28, 2021

Uh oh!

diablodale Aug 28, 2021

Uh oh!

alalek Aug 28, 2021

Uh oh!

diablodale commented Aug 28, 2021

Uh oh!

diablodale commented Aug 28, 2021 •

edited

Loading

Uh oh!

alalek commented Aug 28, 2021

Uh oh!

alalek left a comment •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

diablodale commented Aug 27, 2021 • edited by alalek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

alalek Aug 27, 2021

Choose a reason for hiding this comment

Uh oh!

diablodale Aug 28, 2021

Choose a reason for hiding this comment

Uh oh!

alalek Aug 28, 2021

Choose a reason for hiding this comment

Uh oh!

diablodale Aug 28, 2021

Choose a reason for hiding this comment

Uh oh!

alalek Aug 28, 2021

Choose a reason for hiding this comment

Uh oh!

diablodale commented Aug 28, 2021

No repro

Uh oh!

diablodale commented Aug 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalek commented Aug 28, 2021

Uh oh!

alalek left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

diablodale commented Aug 27, 2021 •

edited by alalek

Loading

diablodale commented Aug 28, 2021 •

edited

Loading

alalek left a comment •

edited

Loading