Skip to content

Fixed Compilation warnings | Issue #16336#16474

Merged
opencv-pushbot merged 1 commit intoopencv:3.4from
gapry:issue_16336
Feb 1, 2020
Merged

Fixed Compilation warnings | Issue #16336#16474
opencv-pushbot merged 1 commit intoopencv:3.4from
gapry:issue_16336

Conversation

@gapry
Copy link
Copy Markdown
Contributor

@gapry gapry commented Jan 31, 2020 •
edited
Loading

This pullrequest changes

resolves #16336

Brief

The main.cpp will occur the compilation warnings. The reason is at the modules/core/include/opencv2/core/types.hpp#L1218

Detailed description

Assume a file calls main.cpp as below

#include <opencv2/core/types.hpp>

int main(void) {
  return 0;
}

If the developer enables the g++ flags -Wall -Wdouble-promotion in the compilation, g++ will output the following warning messages and generate a ELF file.

warning: implicit conversion from ‘float’ to ‘double’ to match other operand of binary expression [-Wdouble-promotion]
 1218 |     return (double)x*pt.x + (double)y*pt.y;
      |            ~~~~~~~~~^~~~~

warning: implicit conversion from ‘float’ to ‘double’ to match other operand of binary expression [-Wdouble-promotion]
 1218 |     return (double)x*pt.x + (double)y*pt.y;
      |                             ~~~~~~~~~^~~~~

If the developer enables the g++ flags -Wall -Werror -Wdouble-promotion in the compilation, g++ will output the same warning messages but can't generate a ELF file.

@gapry gapry mentioned this pull request Jan 31, 2020
Closed
@bduclaux
Copy link
Copy Markdown

bduclaux commented Jan 31, 2020

Sounds good !

gapry
gapry commented Jan 31, 2020
View reviewed changes
modules/core/include/opencv2/core/types.hpp
double Point_<_Tp>::ddot(const Point_& pt) const
{
return (double)x*pt.x + (double)y*pt.y;
return (double)x*(double)(pt.x) + (double)y*(double)(pt.y);
Copy link
Copy Markdown
Contributor Author

@gapry gapry Jan 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Dear @bduclaux

Although the patch can fix the issue, the casting can achieve with static_cast.

Here, it's the code.

return static_cast<double>(x)*static_cast<double>(pt.x) + 
  static_cast<double>(y)*static_cast<double>(pt.y);

In C++ world, I think it's better. Do you agree ?

Copy link
Copy Markdown

@bduclaux bduclaux Jan 31, 2020

Choose a reason for hiding this comment

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

Yes, static_cast is indeed much better.
Btw, there are tons of C-style cast in the code, and it will take months to fully check / rewrite them. Not a problem, unless the compiler emits warnings like above !
Thanks !

@alalek
Copy link
Copy Markdown
Member

alalek commented Jan 31, 2020

Thank you for the contribution!

This patch should go into 3.4 branch first. We will merge changes from 3.4 into master regularly (weekly/bi-weekly).

So, please:

  • change "base" branch of this PR: master => 3.4 (use "Edit" button near PR title)
  • rebase your commits from master onto 3.4 branch. For example:
    git rebase -i --onto upstream/3.4 upstream/master
    (check list of your commits, save and quit (Esc + "wq" + Enter)
    where upstream is configured by following this GitHub guide and fetched (git fetch upstream).
  • push rebased commits into source branch of your fork (with --force option)

Note: no needs to re-open PR, apply changes "inplace".

@gapry gapry changed the base branch from master to 3.4 January 31, 2020 19:28
@gapry
Copy link
Copy Markdown
Contributor Author

gapry commented Jan 31, 2020

@alalek Thank you for your guide. I've done it.

opencv-pushbot pushed a commit that referenced this pull request Feb 1, 2020
@alalek
Merge pull request #16474 from gapry:issue_16336
84c29dc
@opencv-pushbot opencv-pushbot merged commit ac9f8c1 into opencv:3.4 Feb 1, 2020
@alalek alalek mentioned this pull request Feb 1, 2020
Merged
@asmorkalov asmorkalov added the Hackathon https://opencv.org/opencv-hackathon-starts-next-week/ label Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alalek alalek alalek approved these changes

+1 more reviewer

@bduclaux bduclaux bduclaux left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@alalek alalek

Labels

category: build/install Hackathon https://opencv.org/opencv-hackathon-starts-next-week/

Projects

None yet

Milestone

3.4.10

Development

Successfully merging this pull request may close these issues.

5 participants

@gapry @bduclaux @alalek @opencv-pushbot @asmorkalov