Skip to content

Fixed several issues found by static analysis#19620

Merged
opencv-pushbot merged 1 commit intoopencv:masterfrom
mshabunin:static-analysis-issues
Feb 25, 2021
Merged

Fixed several issues found by static analysis#19620
opencv-pushbot merged 1 commit intoopencv:masterfrom
mshabunin:static-analysis-issues

Conversation

@mshabunin
Copy link
Copy Markdown
Contributor

No description provided.

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.

Thank you!

: base(base_), scale(scale_), shift(shift_)
{
CV_Check(base, base == -1.f || base > 0.f, "Unsupported 'base' value");
finalize(); // init normScale and normShift
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It make sense to move finalize "body" here instead (and keep finalize empty).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure, for some reason functors were designed to implement layer's finalize method and all data members of this class are public, so they can be changed externally and then finalized to calculate norm parameters.

cc @SamFC10 , @l-bat

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Only ExpLayer::create() (below) knows about this class.
Base class assumes "updates" through tryFuse() callback, but it is not implemented here. Perhaps even finalize() with empty body is not needed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Isn't it meant to be called here?

virtual void finalize(InputArrayOfArrays, OutputArrayOfArrays) CV_OVERRIDE
{
func.finalize();
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There is already empty implementation in the base class:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK, updated.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My initial thought was to initialize the norm fields here and not have finalize() at all like @alalek suggests. I added it to finalize() instead as the PowerLayer above has a similar finalize() method and thought initializing norm fields in finalize() instead of the constructor "shouldn't" really affect the layer's implementation as finalize() is called before forward() anyway. Now I think its better to shift finalize() body here and remove it.

@asmorkalov
Copy link
Copy Markdown
Contributor

jenkins cn please retry a build

@mshabunin mshabunin force-pushed the static-analysis-issues branch from ead189e to dd59761 Compare February 25, 2021 12:10
@alalek alalek assigned mshabunin and unassigned mshabunin Feb 25, 2021
@opencv-pushbot opencv-pushbot merged commit 37eba84 into opencv:master Feb 25, 2021
@mshabunin mshabunin deleted the static-analysis-issues branch February 25, 2021 20:51
@alalek alalek mentioned this pull request Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

port/backport done Label for maintainers. Authors of PR can ignore this

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants