Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issues spotted by static code analysis and other minor code changes #2

Merged
merged 4 commits into from
Apr 18, 2017

Conversation

irwir
Copy link
Contributor

@irwir irwir commented Jul 23, 2016

Issues spotted by static code analysis

  1. Some header files were not needed
  2. Added initalizer lists (compiles in older versions of Visual Studio unlike initialization in declarations)
  3. Added a number of 'explicit', 'static' and 'const' specifiers
  4. Corrected format specificifiers in PLACEMENT_FMT

Other issues.

  1. It would be safer to use CWnd::FromHandlePermanent() instead of CWnd::FromHandle()
  2. In ResizableLayout::CalcNewChildPosition() pass flags by pointer instead of by reference - simplifies code.
  3. In ResizableGrip create and destroy m_wndGrip automatically. Otherwise debug buids failed in CResizableGrip::UpdateSizeGrip()

1) Some header files were not needed
2) Added initalizer lists (compiles in older versions of Visual Studio unlike initialization in declarations)
3) Added a number of 'explicit', 'static' and 'const' specifiers
4) Corrected format specificifiers in PLACEMENT_FMT

Other issues.
1) It would be safer to use CWnd::FromHandlePermanent() instead of CWnd::FromHandle()
2) In ResizableLayout::CalcNewChildPosition()  pass flags by pointer instead of by reference - simplifies code.
3) In ResizableGrip create and destroy m_wndGrip automatically. Otherwise debug buids failed in CResizableGrip::UpdateSizeGrip()
@ppescher
Copy link
Owner

Thanks for this!
I just have one doubt about creating the CSizeGrip object on the heap. I don't see any real advantage in doing that.. can you explain your point?
Also, I don't see the reason for calling the default constructor in the initializer lists: isn't that done automatically by the compiler?

@irwir
Copy link
Contributor Author

irwir commented Jul 25, 2016

I just have one doubt about creating the CSizeGrip object on the heap.

The code which worked fine with older library (version 1.3) now in debug builds would generate constant stream of asserts.
With the posted changes things got back to normal, but let me think about this a bit more.

I don't see the reason for calling the default constructor in the initializer lists

MS compiler usually fills uninitialized locations with 0xcccccccc values, while default initializer - for example rcOld() should zero-fill the member.
Can you please point out some examples that are most doubtful?

…ically, this point:

3) In ResizableGrip create and destroy m_wndGrip automatically. Otherwise debug buids failed in CResizableGrip::UpdateSizeGrip()
@irwir
Copy link
Contributor Author

irwir commented Jul 26, 2016

Further investigation showed that explicit creation and destruction of grip was not needed, rather the code which used the library needed one line to be commented out. That way it is even better.
Please see my additional commit.

@ppescher
Copy link
Owner

Thanks. What line is to be commented out in the library?

@irwir
Copy link
Contributor Author

irwir commented Jul 27, 2016

No, not in the library.
I wrote about code in the program; there was a call of CreateSizeGrip() method, which was not going well along with the new library.
Do you still need more clarifications on the subject of initializer lists (I posted a question above)?

Enforcing stricter types usage.
Const qualifiers added.
'Safe' input routines.
Re-arranged small parts of code.
@irwir
Copy link
Contributor Author

irwir commented Dec 3, 2016

Also, I don't see the reason for calling the default constructor in the initializer lists: isn't that done automatically by the compiler?

Just in case you forgot these language features: http://en.cppreference.com/w/cpp/language/zero_initialization
Static code analyzers tend to complain about any uninitialized members and variables.

@ppescher
Copy link
Owner

I am willing to merge this pull request, I just have one question about the use of pointer argument in place of a reference (see review).
And sorry for the delay, this is really appreciated. I just don't have much time for my personal projects and tend to live off-line when I'm not at work. ;-)

@irwir
Copy link
Contributor Author

irwir commented Mar 19, 2017

I just have one question about the use of pointer argument in place of a reference (see review).

If the review exists, where could I see it??

On the subject of pointer vs reference.
Essentially both do the same, but:

  • Reference cannot be null. That necessitates creation of temporary variable in GetAnchorPosition().
  • Null pointer is legal and requires no such gimmick.

PS. Sorry, clicked wrong button to post comment. :)

@irwir irwir closed this Mar 19, 2017
@irwir irwir reopened this Mar 19, 2017
@ppescher
Copy link
Owner

I thought the code review was visible... I can see it in this thread too.

I was about the change from reference to pointer in CalcNewChildPosition. My point was that reference i generally easier to use (no extra * for every access to the variable), moreover I didn't have a reason for not getting the flags along with the calculated position, so I don't see a use for a NULL pointer.

@irwir
Copy link
Contributor Author

irwir commented Mar 20, 2017

I thought the code review was visible... I can see it in this thread too.

I see only comments, no reviews; and don't know why.

reference i generally easier to use (no extra * for every access to the variable)

The optional parameter in GetAnchorPosition makes it the opposite.
Smaller and more straightforward code for the price of lke 5 asterisks is a good bargain.

moreover I didn't have a reason for not getting the flags along with the calculated position, so I don't see a use for a NULL pointer.

The flags parameter in GetAnchorPosition is optional.
Should it be different, then reference would have been preferable.

@@ -673,19 +667,19 @@ BOOL CResizableLayout::LikesClipping(const LAYOUTINFO& layout) const
* control in the layout and flags for @c SetWindowPos
*/
void CResizableLayout::CalcNewChildPosition(const LAYOUTINFO& layout,
const CRect &rectParent, CRect &rectChild, UINT& uFlags) const
const CRect &rectParent, CRect &rectChild, UINT *lpFlags) const
Copy link
Owner

Choose a reason for hiding this comment

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

What is the reason for the change to the last argument type?
I like the reference because I don't have to write extra * each time I want to access the variable. Looks like a bit overkill to me to have a pointer here.

@ppescher
Copy link
Owner

My bad, the review was still "pending"... I thought it was automatically visible once added.

I will merge the changes.

@ppescher ppescher closed this Apr 18, 2017
@ppescher ppescher reopened this Apr 18, 2017
@ppescher ppescher merged commit 72d1884 into ppescher:master Apr 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants