Skip to content

GAPI: fix Clang warning for dangling references in for loop variable#19050

Merged
opencv-pushbot merged 1 commit intoopencv:masterfrom
anton-potapov:fix_range_var_clang_warnings
Jan 14, 2021
Merged

GAPI: fix Clang warning for dangling references in for loop variable#19050
opencv-pushbot merged 1 commit intoopencv:masterfrom
anton-potapov:fix_range_var_clang_warnings

Conversation

@anton-potapov
Copy link
Copy Markdown
Contributor

@anton-potapov anton-potapov commented Dec 8, 2020 •
edited
Loading

Clang complains that the code in question binds a temporary value (returned by indexed) to a (const) reference. While technically assigning a temporary value to const reference is OK (as it extends a lifetime of the temporary to lifetime of the reference), let\s please the compiler :)

resolves #18928

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
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake
buildworker:Mac=macosx-1
buildworker:iOS=macosx-1

force_builders=Custom,Custom Win,Custom Mac
build_gapi_standalone:Linux x64=ade-0.1.1f
build_gapi_standalone:Win64=ade-0.1.1f
build_gapi_standalone:Mac=ade-0.1.1f
build_gapi_standalone:Linux x64 Debug=ade-0.1.1f

build_image:Custom=centos:7
buildworker:Custom=linux-1
build_gapi_standalone:Custom=ade-0.1.1f

test_modules:Custom=gapi
test_modules:Custom Win=gapi
test_modules:Custom Mac=gapi

buildworker:Custom=linux-1
# disabled due high memory usage: test_opencl:Custom=ON
test_opencl:Custom=OFF
test_bigdata:Custom=1
test_filter:Custom=*

@anton-potapov anton-potapov force-pushed the fix_range_var_clang_warnings branch from 9901212 to da5bfef Compare December 8, 2020 11:53
alalek
alalek approved these changes Dec 8, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

@anton-potapov
Copy link
Copy Markdown
Contributor Author

anton-potapov commented Dec 9, 2020

@dmatveev, could you please take a look ?

dmatveev
dmatveev approved these changes Dec 11, 2020
View reviewed changes
modules/gapi/src/backends/common/gcompoundkernel.cpp
{
m_args.resize(in_args.size());
for (const auto& it : ade::util::indexed(in_args))
for (const auto it : ade::util::indexed(in_args))
Copy link
Copy Markdown
Contributor

@dmatveev dmatveev Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out of curiosity: y not auto && it ?

Copy link
Copy Markdown
Member

@alalek alalek Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

details about that approach: https://stackoverflow.com/questions/13130708/what-is-the-advantage-of-using-forwarding-references-in-range-based-for-loops

Copy link
Copy Markdown
Contributor

@dmatveev dmatveev Dec 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anton-potapov ?

Copy link
Copy Markdown
Contributor Author

@anton-potapov anton-potapov Jan 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this context both versions are equal and correct, however "plain copy" version put less stress on the reader (IMHO) more explicitly describing the intent

@anton-potapov
Copy link
Copy Markdown
Contributor Author

anton-potapov commented Jan 14, 2021

@alalek could you please merge it ?

@opencv-pushbot opencv-pushbot merged commit 5202186 into opencv:master Jan 14, 2021
@alalek alalek mentioned this pull request Jan 19, 2021
Merged
@alalek alalek mentioned this pull request Apr 9, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dmatveev dmatveev dmatveev approved these changes

@alalek alalek alalek approved these changes

Assignees

@dmatveev dmatveev

Labels

bug category: build/install category: g-api / gapi

Projects

None yet

Milestone

4.5.2

Development

Successfully merging this pull request may close these issues.

G-API: "range for" result reference warning near ade::util::indexed()

4 participants

@anton-potapov @dmatveev @alalek @opencv-pushbot