search

google / UIforETW

User interface for recording and managing ETW traces
https://randomascii.wordpress.com/2015/04/14/uiforetw-windows-performance-made-easier/
Apache License 2.0
1.59k stars 201 forks source link

changed `assert`s to `ATLASSERT`s #25

Closed ariccio closed 9 years ago

ariccio commented 9 years ago

I also inserted _Pre_satisfies_ annotations for function and object/member function preconditions.

In Settings.cpp, I changed an implicit conversion of *CSettings/*this to HWND (via CWnd::operator HWND) to explicitly passing m_hWnd.

ATLASSERT calls _CrtDbgBreak instead of abort, and is generally suited better to UI programming.

randomascii commented 9 years ago

Seems fairly reasonable, but I made a few comments.

randomascii commented 9 years ago

Let me know when it's ready and I'll merge. Ditto for the other one.

ariccio commented 9 years ago

I made the changes that (I think) you wanted.

I also added two _Pre_satisfies_(nCode == HC_ACTION) where we ATLASSERTed function preconditions.

Yeah, UIETWASSERT is a bit of overengineering, but it doesn't really detract from readability (custom assert macros are pretty common), and it opens up options for later.

Did you want me to remove m_hWnd?

randomascii commented 9 years ago

I've come around to m_hWnd. I still think it is abhorrent that it is accessible directly, but I can see the advantages of referencing m_hWnd over *this, even though GetHwnd() would be better than either.

On Sun, May 10, 2015 at 11:17 PM, Alexander Riccio <notifications@github.com

wrote:

I made the changes that (I think) you wanted.

I also added two _Presatisfies(nCode == HC_ACTION) where we ATLASSERTed function preconditions.

Yeah, UIETWASSERT is a bit of overengineering, but it doesn't really detract from readability (custom assert macros are pretty common), and it opens up options for later.

Did you want me to remove m_hWnd?

— Reply to this email directly or view it on GitHub https://github.com/google/UIforETW/pull/25#issuecomment-100785263.

Bruce Dawson

randomascii commented 9 years ago

I just did a debug /analyze build and I got a lot of warnings which seemed to be related to this change. Most of them are on calls to SmartEnableWindow. The warnings happen on 32-bit and 64-bit.

c:\github\uiforetw\uiforetw\settings.cpp(147): warning C6388: 'm_hWnd' might not be '0': this does not adhere to the specification for the function 'SHGetSpecialFolderPathW'. c:\github\uiforetw\uiforetw\utility.h(43): warning C28220: Integer expression expected for annotation 'SAL_satisfies', parameter 1, found '__formal(0,Win)'. c:\github\uiforetw\uiforetw\utility.cpp(209): warning C28220: [Warning applies to SmartEnableWindow at c:\github\uiforetw\uiforetw\utility.h(43)]Integer expression expected for annotation 'SAL_satisfies', parameter 1, found '__formal(0,Win)'. c:\github\uiforetw\uiforetw\utility.cpp(209): warning C28020: The expression '0' is not true at this call. c:\github\uiforetw\uiforetw\uiforetwdlg.cpp(358): warning C28020: The expression '0' is not true at this call. c:\github\uiforetw\uiforetw\uiforetwdlg.cpp(384): warning C28020: The expression '0' is not true at this call. c:\github\uiforetw\uiforetw\uiforetwdlg.cpp(553): warning C28020: The expression '0' is not true at this call. c:\github\uiforetw\uiforetw\uiforetwdlg.cpp(554): warning C28020: The expression '0' is not true at this call. c:\github\uiforetw\uiforetw\uiforetwdlg.cpp(555): warning C28020: The expression '0' is not true at this call. c:\github\uiforetw\uiforetw\uiforetwdlg.cpp(556): warning C28020: The expression '0' is not true at this call. c:\github\uiforetw\uiforetw\uiforetwdlg.cpp(558): warning C28020: The expression '0' is not true at this call. c:\github\uiforetw\uiforetw\uiforetwdlg.cpp(559): warning C28020: The expression '0' is not true at this call. c:\github\uiforetw\uiforetw\uiforetwdlg.cpp(560): warning C28020: The expression '0' is not true at this call. c:\github\uiforetw\uiforetw\uiforetwdlg.cpp(1200): warning C28020: The expression '0' is not true at this call. c:\github\uiforetw\uiforetw\uiforetwdlg.cpp(1208): warning C28020: The expression '0' is not true at this call.

ariccio commented 9 years ago

Strangely, changing _Pre_satisfies_(Win) to _Pre_satisfies_(Win != NULL) fixes those.

I'll submit a patch with that change in a minute.

randomascii commented 9 years ago

/analyze has a long history of not handling boolean expressions robustly. I'm glad that fix works.

Does it find bugs?

On Tue, May 12, 2015 at 4:23 PM, Alexander Riccio notifications@github.com wrote:

Strangely, changing _Presatisfies(Win) to _Presatisfies(Win != NULL) fixes those.

I'll submit a patch with that change in a minute.

— Reply to this email directly or view it on GitHub https://github.com/google/UIforETW/pull/25#issuecomment-101458324.

Bruce Dawson

ariccio commented 9 years ago

maybe

I'm not sure if that's a bug or not, yet.

ariccio commented 9 years ago

Sidenote: MFC really goes overboard on using macro expansion to add a layer of "abstraction" to message maps: ON_BN_CLICKED(IDC_COPYSTARTUPPROFILE, &CSettings::OnBnClickedCopystartupprofile) is expanded to ON_CONTROL(BN_CLICKED, IDC_COPYSTARTUPPROFILE, &CSettings::OnBnClickedCopystartupprofile), which is further expanded to { WM_COMMAND, (WORD)BN_CLICKED, (WORD)id, (WORD)IDC_COPYSTARTUPPROFILE, AfxSigCmd_v, (static_cast< AFX_PMSG > (&CSettings::OnBnClickedCopystartupprofile)) }

...which makes it really fun to try to track the usage/initialization of m_hWnd

randomascii commented 9 years ago

Spoiler alert: it's set when the underlying window is created.

On Tue, May 12, 2015 at 4:40 PM, Alexander Riccio notifications@github.com wrote:

Sidenote: MFC really goes overboard on using macro expansion to add a layer of "abstraction" to message maps: ON_BN_CLICKED(IDC_COPYSTARTUPPROFILE, &CSettings::OnBnClickedCopystartupprofile) is expanded to ON_CONTROL(BN_CLICKED, IDC_COPYSTARTUPPROFILE, &CSettings::OnBnClickedCopystartupprofile), which is further expanded to { WM_COMMAND, (WORD)BN_CLICKED, (WORD)id, (WORD)IDC_COPYSTARTUPPROFILE, AfxSigCmd_v, (static_cast< AFX_PMSG > (&CSettings::OnBnClickedCopystartupprofile)) }

...which makes it really fun to try to track the usage/initialization of m_hWnd

— Reply to this email directly or view it on GitHub https://github.com/google/UIforETW/pull/25#issuecomment-101460723.

Bruce Dawson

ariccio commented 9 years ago

Spoiler alert: it's set when the underlying window is created.

I figured that, but I'd like to make sure that MFC/CWnd isn't doing anything wacky while we're not looking.

The fact that analyze is saying that it might be zero, combined with the fact that CWnd provides the (mostly useless) GetSafeHwnd member function makes me suspicious.

I'd forgotten about GetSafeHwnd, because there are really an absurd number of member functions in CWnd.

randomascii commented 9 years ago

Never make the mistake of thinking that /analyze's warnings are reasonable.

Sometimes they are bizarrely accurate but, unfortunately, if a skilled developer cannot find the bug quickly then it is usually because /analyze is insane.

Knowing it's patterns for brilliance versus insanity helps, and I hate to counsel people to ignore it, but sometimes you have to. Sigh...

On Tue, May 12, 2015 at 4:51 PM, Alexander Riccio notifications@github.com wrote:

Spoiler alert: it's set when the underlying window is created.

I figured that, but I'd like to make sure that MFC/CWnd isn't doing anything wacky while we're not looking.

The fact that analyze is saying that it might be zero, combined with the fact that CWnd provides the (mostly useless) GetSafeHwnd member function makes me suspicious.

I'd forgotten about GetSafeHwnd, because there are really an absurd number of member functions in CWnd.

— Reply to this email directly or view it on GitHub https://github.com/google/UIforETW/pull/25#issuecomment-101462020.

Bruce Dawson