ppescher / resizablelib

A set of MFC classes to easily make resizable windows
Other
55 stars 27 forks source link

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

Closed irwir closed 7 years ago

irwir commented 7 years ago

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()

ppescher commented 7 years ago

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 commented 7 years ago

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?

irwir commented 7 years ago

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 commented 7 years ago

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

irwir commented 7 years ago

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)?

irwir commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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:

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

ppescher commented 7 years ago

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 commented 7 years ago

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.

ppescher commented 7 years ago

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

I will merge the changes.