Removed all pre-C++11 code, workarounds, and branches by seanm · Pull Request #23736 · opencv/opencv

seanm · 2023-06-02T16:17:13Z

This removes a bunch of pre-C++11 workrarounds that are no longer necessary as C++11 is now required.

It is a nice clean up and simplification.

force_builders=Custom
build_image:Custom=centos:7
buildworker:Custom=linux-1

vpisarev · 2023-06-06T09:53:15Z

@seanm, I welcome those changes and believe something like this should be done. But maybe we should target 5.x first and do it slightly more aggressive, including C++ 14 and/or C++ 17 features.

Also, one of the things where some of our hacks should be removed in favor of a standard C++ feature is singletons. We have quite a few of them and could possibly replace them with std::call_once + std::once_flag.

@opencv-alalek, what do you think?

seanm · 2023-06-06T15:32:44Z

But maybe we should target 5.x first...

Why? 4.x already has #error "OpenCV 4.x+ requires enabled C++11 support". This PR doesn't increase any requirements. All the code removed is already dead because of that existing #error.

...do it slightly more aggressive, including C++ 14 and/or C++ 17 features

That's a whole other discussion. I'm not looking to drop C++11, I'm looking to drop C++03 and older. Increasing the minimum language dialect requires wider discussion I'd imagine, but I'm not at all involved in this project's governance...

vpisarev · 2023-06-09T10:10:12Z

the major reason is that until 5.0 is released, we backport some patches to 3.4, which does not require C++ 11.

For 5.0 we already decided to make C++ 14 the minimum required standard. Maybe we will raise it up to C++ 17, we will see.

seanm · 2023-06-12T14:46:03Z

Well, I disagree, but it's up to you. I assume github allows you to merge this into either branch?

seanm · 2023-06-29T13:55:27Z

Do I have to rebase my commits onto the 5.x branch? Or can everything be done from github?

mshabunin · 2023-07-07T17:31:58Z

This patch breaks windows builds and documentation build:

/build/precommit_docs/4.x/opencv/doc/tutorials/core/how_to_use_OpenCV_parallel_for_/how_to_use_OpenCV_parallel_for_.markdown:174: warning: block marked with [mandelbrot-parallel-call] for \snippet should appear twice in file how_to_use_OpenCV_parallel_for_.cpp, found it 0 times
/build/precommit_docs/4.x/opencv/doc/tutorials/core/how_to_use_OpenCV_parallel_for_new/how_to_use_OpenCV_parallel_for_new.markdown:107: warning: block marked with [convolution-parallel] for \snippet should appear twice in file how_to_use_OpenCV_parallel_for_new.cpp, found it 0 times
/build/precommit_docs/4.x/opencv/doc/tutorials/core/how_to_use_OpenCV_parallel_for_new/how_to_use_OpenCV_parallel_for_new.markdown:114: warning: block marked with [overload-full] for \snippet should appear twice in file how_to_use_OpenCV_parallel_for_new.cpp, found it 0 times
/build/precommit_docs/4.x/opencv/doc/tutorials/core/how_to_use_OpenCV_parallel_for_new/how_to_use_OpenCV_parallel_for_new.markdown:117: warning: block marked with [convolution-parallel-function] for \snippet should appear twice in file how_to_use_OpenCV_parallel_for_new.cpp, found it 0 times
/build/precommit_docs/4.x/opencv/doc/tutorials/core/how_to_use_OpenCV_parallel_for_new/how_to_use_OpenCV_parallel_for_new.markdown:122: warning: block marked with [overload-row-split] for \snippet should appear twice in file how_to_use_OpenCV_parallel_for_new.cpp, found it 0 times
/build/precommit_docs/4.x/opencv/doc/tutorials/core/how_to_use_OpenCV_parallel_for_new/how_to_use_OpenCV_parallel_for_new.markdown:125: warning: block marked with [convolution-parallel-function-row] for \snippet should appear twice in file how_to_use_OpenCV_parallel_for_new.cpp, found it 0 times

seanm · 2023-07-07T20:02:19Z

This patch breaks windows builds and documentation build:

I think I've just fixed the doc build...

mshabunin · 2023-07-10T07:46:23Z

There are still issues with Windows build (Visual Studio 2019 Developer Command Prompt v16.11.21)

C:\build\precommit_windows64\4.x\opencv\modules\core\include\opencv2/core/cvdef.h(752): 
fatal error C1189: #error:  "OpenCV 4.x+ requires enabled C++11 support" 
(compiling source file C:\build\precommit_windows64\4.x\opencv\modules\imgproc\src\imgwarp.avx2.cpp)

Perhaps we need some special handling for MSVC - https://devblogs.microsoft.com/cppblog/msvc-now-correctly-reports-__cplusplus/ , but if we choose this way, we can still break user's aplications with our cvdef.hpp. So alternatively we need to use old _MSVC_LANG macro (https://learn.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=msvc-170#microsoft-specific-predefined-macros). Example from vulkan header:

 #if (defined(__cplusplus) && (__cplusplus >= 201103L)) || (defined(_MSVC_LANG) && (_MSVC_LANG >= 201103L))

seanm · 2023-07-10T19:51:57Z

Perhaps we need some special handling for MSVC - https://devblogs.microsoft.com/cppblog/msvc-now-correctly-reports-__cplusplus/ , but if we choose this way, we can still break user's aplications with our cvdef.hpp. So alternatively we need to use old _MSVC_LANG macro (https://learn.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=msvc-170#microsoft-specific-predefined-macros). Example from vulkan header:

Interesting. I've updated my commit with something like the vulkan header.

I don't have Windows though, so this may take me a few trys with the help of github CI...

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

mshabunin · 2023-08-11T19:17:48Z

Good, ifdef part is passing now, but several compilation issues have risen in cap_msmf and some opencv_contrib modules.
https://pullrequest.opencv.org/buildbot/builders/precommit_windows64/builds/102452/steps/compile%20release/logs/stdio
😮‍💨 I hope that most of them can be resolved by including correct headers or minor modifications, but there is possibility that some parts need to be rewritten to comply with this picky compiler and its C++ standard vision 🙂 .

seanm · 2023-08-11T22:25:33Z

Good, ifdef part is passing now, but several compilation issues have risen in cap_msmf and some opencv_contrib modules. https://pullrequest.opencv.org/buildbot/builders/precommit_windows64/builds/102452/steps/compile%20release/logs/stdio 😮‍💨 I hope that most of them can be resolved by including correct headers or minor modifications, but there is possibility that some parts need to be rewritten to comply with this picky compiler and its C++ standard vision 🙂 .

Seems most are due to missing headers, due to me removing the include in cvdef.h. I'll fix them.

But some are from opencv_contrib which seems to be a different repro. I can fix them too, but I wonder how much other code out there is expecting to be automatically included...

seanm · 2023-08-16T15:14:56Z

Actually, by including <array> in the appropriate .pch files I think I've fixed the remaining issues... CI seems to be green now.

…itly where needed

…pat defines

...utorials/core/how_to_use_OpenCV_parallel_for_new/how_to_use_OpenCV_parallel_for_new.markdown

asmorkalov · 2023-11-09T12:26:49Z

doc/tutorials/core/how_to_use_OpenCV_parallel_for_/how_to_use_OpenCV_parallel_for_.markdown

-Here, the range represents the total number of operations to be executed, so the total number of pixels in the image.
-To set the number of threads, you can use: @ref cv::setNumThreads. You can also specify the number of splitting using the
-nstripes parameter in @ref cv::parallel_for_. For instance, if your processor has 4 threads, setting `cv::setNumThreads(2)`
-or setting `nstripes=2` should be the same as by default it will use all the processor threads available but will split the
-workload only on two threads.


Please restore setNumThreads related description.

This allowed removing a lot of dead code. Updated some documentation consequently.

asmorkalov · 2023-11-11T06:02:30Z

Documentation warnings on Buildbot:

/build/precommit_docs/4.x/opencv/doc/tutorials/core/how_to_use_OpenCV_parallel_for_new/how_to_use_OpenCV_parallel_for_new.markdown:114: warning: block marked with [overload-full] for \snippet should appear twice in file how_to_use_OpenCV_parallel_for_new.cpp, found it 0 times
/build/precommit_docs/4.x/opencv/doc/tutorials/core/how_to_use_OpenCV_parallel_for_new/how_to_use_OpenCV_parallel_for_new.markdown:117: warning: block marked with [convolution-parallel-function] for \snippet should appear twice in file how_to_use_OpenCV_parallel_for_new.cpp, found it 0 times
/build/precommit_docs/4.x/opencv/doc/tutorials/core/how_to_use_OpenCV_parallel_for_new/how_to_use_OpenCV_parallel_for_new.markdown:122: warning: block marked with [overload-row-split] for \snippet should appear twice in file how_to_use_OpenCV_parallel_for_new.cpp, found it 0 times
/build/precommit_docs/4.x/opencv/doc/tutorials/core/how_to_use_OpenCV_parallel_for_new/how_to_use_OpenCV_parallel_for_new.markdown:125: warning: block marked with [convolution-parallel-function-row] for \snippet should appear twice in file how_to_use_OpenCV_parallel_for_new.cpp, found it 0 times

seanm · 2023-11-11T14:25:17Z

Documentation warnings on Buildbot:

Yeah, that's because I brought back the comments you asked me to. :) CI was green before.

asmorkalov · 2023-11-16T13:51:32Z

...utorials/core/how_to_use_OpenCV_parallel_for_new/how_to_use_OpenCV_parallel_for_new.markdown


 @note Although values of a pixel in a particular stripe may depend on pixel values outside the stripe, these are only read only operations and hence will not cause undefined behaviour.

+C++11 standard allows a parallel implementation with a lambda expression:


The following documentation describes operator() overloading and other steps of the old version. I propose not to remove pre-C++11 code, but just remove ifdefs for C++11.

asmorkalov · 2023-11-16T13:52:05Z

...pp/tutorial_code/core/how_to_use_OpenCV_parallel_for_/how_to_use_OpenCV_parallel_for_new.cpp

-        sz = kernel.rows / 2;
-    }
-
-    //! [overload-full]


There is reference to overload-full block in markdown.

asmorkalov · 2023-11-16T13:52:34Z

...pp/tutorial_code/core/how_to_use_OpenCV_parallel_for_/how_to_use_OpenCV_parallel_for_new.cpp

-    //! [convolution-parallel-function]
-}
-
-//! [conv-parallel-row-split]


asmorkalov · 2023-11-16T13:52:42Z

...pp/tutorial_code/core/how_to_use_OpenCV_parallel_for_/how_to_use_OpenCV_parallel_for_new.cpp

-    int sz = kernel.rows / 2;
-    copyMakeBorder(src, src, sz, sz, sz, sz, BORDER_REPLICATE);
-
-    //! [convolution-parallel-function]


asmorkalov · 2023-11-16T13:52:55Z

...pp/tutorial_code/core/how_to_use_OpenCV_parallel_for_/how_to_use_OpenCV_parallel_for_new.cpp

-        sz = kernel.rows / 2;
-    }
-
-    //! [overload-row-split]


The same. See CI warnings.

asmorkalov · 2024-01-10T11:21:04Z

@seanm friendly reminder.

seanm · 2024-01-10T17:28:15Z

@seanm friendly reminder.

I don't understand what it is you want to do in those documentation files. And I'm not familiar with the file's syntax nor the meaning of those tags. Nor am I familiar with the classes the document is describing. I think I give up at this point. Feel free to take what I've started and continue it.

doc/tutorials/core/how_to_use_OpenCV_parallel_for_/how_to_use_OpenCV_parallel_for_.markdown

mshabunin · 2024-01-19T11:29:22Z

@asmorkalov , tutorial_how_to_use_OpenCV_parallel_for_ is considered obsolete and is kept only for link preservation (as I understand). It is not listed in the corresponding section: https://docs.opencv.org/4.x/de/d7a/tutorial_table_of_content_core.html

tutorial_how_to_use_OpenCV_parallel_for_new is the new and updated tutorial.

See #20361

seanm · 2024-01-19T14:33:19Z

I thought you didn't want this merged until 5.x?

mshabunin · 2024-01-19T15:51:09Z

3.4 branch is considered EOL now (https://github.com/opencv/opencv/wiki/Branches), so we don't need to do backporting anymore (it was the main concern originally). And forward porting to 5.x will be done by the OpenCV team on a regular basis.

I believe there is still no decision on minimal C++ standard for 5.x.

Removed all pre-C++11 code, workarounds, and branches opencv#23736 This removes a bunch of pre-C++11 workrarounds that are no longer necessary as C++11 is now required. It is a nice clean up and simplification. * No longer unconditionally #include <array> in cvdef.h, include explicitly where needed * Removed deprecated CV_NODISCARD, already unused in the codebase * Removed some pre-C++11 workarounds, and simplified some backwards compat defines * Removed CV_CXX_STD_ARRAY * Removed CV_CXX_MOVE_SEMANTICS and CV_CXX_MOVE * Removed all tests of CV_CXX11, now assume it's always true. This allowed removing a lot of dead code. * Updated some documentation consequently. * Removed all tests of CV_CXX11, now assume it's always true * Fixed links. --------- Co-authored-by: Maksim Shabunin <maksim.shabunin@gmail.com> Co-authored-by: Alexander Smorkalov <alexander.smorkalov@xperience.ai>

asmorkalov requested a review from opencv-alalek June 3, 2023 07:34

vpisarev self-requested a review June 6, 2023 09:49

seanm force-pushed the c++11-simplifications branch 2 times, most recently from f69dcc4 to c27f024 Compare July 10, 2023 03:43

mshabunin mentioned this pull request Jul 10, 2023

Fix compiler warnings for GCC 11-12 and Clang 13 #23938

Merged

seanm force-pushed the c++11-simplifications branch from c27f024 to 5cfe4de Compare July 10, 2023 19:53

seanm changed the title ~~C++11 simplifications~~ Removed all pre-C++11 code, workarounds, and branches Jul 10, 2023

mshabunin reviewed Jul 11, 2023

View reviewed changes

modules/core/include/opencv2/core/cvdef.h Outdated Show resolved Hide resolved

seanm force-pushed the c++11-simplifications branch from 5cfe4de to de976c3 Compare August 11, 2023 13:15

mshabunin reviewed Aug 11, 2023

View reviewed changes

modules/core/include/opencv2/core/cvdef.h Show resolved Hide resolved

seanm force-pushed the c++11-simplifications branch from de976c3 to 4ab4fd0 Compare August 11, 2023 16:00

seanm force-pushed the c++11-simplifications branch from 4ab4fd0 to 301c9b8 Compare August 12, 2023 15:55

mshabunin mentioned this pull request Aug 24, 2023

Fix compilation when forcing later C++. #24194

Merged

3 tasks

seanm force-pushed the c++11-simplifications branch from 301c9b8 to 5dc01fd Compare September 8, 2023 17:25

seanm force-pushed the c++11-simplifications branch from 5dc01fd to bf4b6ca Compare October 6, 2023 14:36

seanm added 3 commits November 9, 2023 17:54

No longer unconditionally #include <array> in cvdef.h, include explic…

3b14616

…itly where needed

Removed deprecated CV_NODISCARD, already unused in the codebase

6b8c0d2

Removed some pre-C++11 workarounds, and simplified some backwards com…

229fe48

…pat defines

seanm added 2 commits November 9, 2023 17:54

Removed CV_CXX_STD_ARRAY

7bf6b24

Removed CV_CXX_MOVE_SEMANTICS and CV_CXX_MOVE

2827433

seanm force-pushed the c++11-simplifications branch from bf4b6ca to eb91f6b Compare November 9, 2023 22:55

asmorkalov reviewed Nov 10, 2023

View reviewed changes

vpisarev approved these changes Nov 10, 2023

View reviewed changes

Removed all tests of CV_CXX11, now assume it's always true

0d71e26

This allowed removing a lot of dead code. Updated some documentation consequently.

seanm force-pushed the c++11-simplifications branch from eb91f6b to 0d71e26 Compare November 10, 2023 19:29

asmorkalov reviewed Nov 16, 2023

View reviewed changes

mshabunin added 2 commits January 15, 2024 12:01

Merge branch '4.x' into pr23736

7c89b4c

fixup! Removed all tests of CV_CXX11, now assume it's always true

264a76b

mshabunin requested a review from asmorkalov January 16, 2024 10:06

Fixed links.

009e6bd

asmorkalov approved these changes Jan 19, 2024

View reviewed changes

doc/tutorials/core/how_to_use_OpenCV_parallel_for_/how_to_use_OpenCV_parallel_for_.markdown Show resolved Hide resolved

doc/tutorials/core/how_to_use_OpenCV_parallel_for_/how_to_use_OpenCV_parallel_for_.markdown Outdated Show resolved Hide resolved

asmorkalov merged commit e64857c into opencv:4.x Jan 19, 2024

asmorkalov mentioned this pull request Jan 23, 2024

5.x merge 4.x #24912

Merged


		@note Although values of a pixel in a particular stripe may depend on pixel values outside the stripe, these are only read only operations and hence will not cause undefined behaviour.

		C++11 standard allows a parallel implementation with a lambda expression:

Uh oh!

Conversation

seanm commented Jun 2, 2023 • edited by opencv-alalek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vpisarev commented Jun 6, 2023

Uh oh!

seanm commented Jun 6, 2023

Uh oh!

vpisarev commented Jun 9, 2023

Uh oh!

seanm commented Jun 12, 2023

Uh oh!

seanm commented Jun 29, 2023

Uh oh!

mshabunin commented Jul 7, 2023

Uh oh!

seanm commented Jul 7, 2023

Uh oh!

mshabunin commented Jul 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seanm commented Jul 10, 2023

Uh oh!

Uh oh!

Uh oh!

mshabunin commented Aug 11, 2023

Uh oh!

seanm commented Aug 11, 2023

Uh oh!

seanm commented Aug 16, 2023

Uh oh!

Uh oh!

asmorkalov Nov 9, 2023

Choose a reason for hiding this comment

Uh oh!

seanm Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

asmorkalov commented Nov 11, 2023

Uh oh!

seanm commented Nov 11, 2023

Uh oh!

asmorkalov Nov 16, 2023

Choose a reason for hiding this comment

Uh oh!

asmorkalov Nov 16, 2023

Choose a reason for hiding this comment

Uh oh!

asmorkalov Nov 16, 2023

Choose a reason for hiding this comment

Uh oh!

asmorkalov Nov 16, 2023

Choose a reason for hiding this comment

Uh oh!

asmorkalov Nov 16, 2023

Choose a reason for hiding this comment

Uh oh!

asmorkalov commented Jan 10, 2024

Uh oh!

seanm commented Jan 10, 2024

Uh oh!

Uh oh!

Uh oh!

mshabunin commented Jan 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seanm commented Jan 19, 2024

Uh oh!

mshabunin commented Jan 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

seanm commented Jun 2, 2023 •

edited by opencv-alalek

Loading

mshabunin commented Jul 10, 2023 •

edited

Loading

mshabunin commented Jan 19, 2024 •

edited

Loading