[haiku-commits] Re: haiku: hrev45802 - in src: apps/debugger/user_interface/gui/team_window kits/interface

  • From: Ingo Weinhold <ingo_weinhold@xxxxxx>
  • To: haiku-commits@xxxxxxxxxxxxx
  • Date: Sat, 29 Jun 2013 18:25:10 +0200

On 06/29/2013 05:56 PM, anevilyak@xxxxxxxxx wrote:

@@ -149,16 +157,23 @@ StackTraceView::SetStackTrace(StackTrace* stackTrace)
  {
        if (stackTrace == fStackTrace)
                return;
-
-       if (fStackTrace != NULL)
-               fStackTrace->ReleaseReference();
-
-       fStackTrace = stackTrace;
-
-       if (fStackTrace != NULL)
-               fStackTrace->AcquireReference();
-
-       fFramesTableModel->SetStackTrace(fStackTrace);
+       else if (stackTrace == NULL) {


Since the "if" block returns, there shouldn't be an "else".


+               if (fTraceUpdateRunner == NULL) {
+                       BMessage message(MSG_CLEAR_STACK_TRACE);
+                       message.AddPointer("currentTrace", fStackTrace);
+                       fTraceUpdateRunner = new(std::nothrow) 
BMessageRunner(this,
+                               message, 250000, 1);
+                       if (fTraceUpdateRunner != NULL
+                               && fTraceUpdateRunner->InitCheck() != B_OK) {
+                               delete fTraceUpdateRunner;
+                               fTraceUpdateRunner = NULL;
+                       }
+               }
+       } else {
+               delete fTraceUpdateRunner;
+               fTraceUpdateRunner = NULL;
+               _SetStackTrace(stackTrace);
+       }
  }


I would organize the control flow like this:

        if (stackTrace == NULL) {
                if (fTraceUpdateRunner != NULL)
                        return;

                BMessage message(MSG_CLEAR_STACK_TRACE);
                message.AddPointer("currentTrace", fStackTrace);
                fTraceUpdateRunner = new(std::nothrow) BMessageRunner(this,
                        message, 250000, 1);
                if (fTraceUpdateRunner != NULL
                        && fTraceUpdateRunner->InitCheck() == B_OK) {
                        return;
                }
        }

        delete fTraceUpdateRunner;
        fTraceUpdateRunner = NULL;
        _SetStackTrace(stackTrace);
This avoids (a small) code duplication and also calls _SetStackTrace() in case the message runner couldn't be created.
The deletion and resetting of fTraceUpdateRunner could be moved to _SetStackTrace(). That would also fix the problem that that doesn't happen in MessageReceived() yet. 


@@ -205,6 +220,29 @@ StackTraceView::SaveSettings(BMessage& settings)


  void
+StackTraceView::MessageReceived(BMessage* message)
+{
+       switch (message->what) {
+               case MSG_CLEAR_STACK_TRACE:
+               {
+                       StackTrace* currentStackTrace;
+                       if (message->FindPointer("currentTrace",
+                                       
reinterpret_cast<void**>(&currentStackTrace))
+                                       == B_OK && currentStackTrace == 
fStackTrace) {


Why not static_cast (or C cast)?

For aesthetical reasons I'd break the last line before the "&&".
Well, actually I wouldn't bother with the "currentTrace" field at all, but rather reduce the check to "fTraceUpdateRunner != NULL". In theory an old message could thus reset the stack trace prematurely, but that's neither all that likely nor harmful. Besides that the stack trace object comparison isn't much better, since a new object could just have been allocated at the same address as the old one (not at all unlikely). 


+void
  StackTraceView::TableSelectionChanged(Table* table)
  {
Some additional handling might needed here. Not that many users will be fast enough to select another stack frame in the short time window, but we can't take chances ... :-) 

CU, Ingo

Other related posts:

  1. Home
  2. haiku-commits
  3. Archive
  4. 06-2013