Author Topic: Rev9537 introduce a bug which will two class labels in the symbol browser  (Read 6946 times)

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5362
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
@Alpha and all.

The steps to reproduce the bug:

1, set the symbol browser view option to show "current file's tokens"
2, open Codeblocks.cbp, let the parser finish its job
3, Open a header file, such as: plugins\codecompletion\classbrowserbuilderthread.h
4, edit the header file, and save it
5, see the updated symbol browser tree, you can see duplicated class labels.

Reason:
Rev9537 add inherit information for the class, saved in m_Args, but once a file get reparsed, it first try to delete the class definition token, since the class token shared between h/cpp, this does not actually remove the token, just adjust the m_File and m_Line info. Later when parsing the header, the class token should be reused. After the rev9537, the bold line is not true any more.

Code
    // none of the above; check for token under parent
    if (!newToken)
    {
        newToken = TokenExists(newname, baseArgs, m_LastParent, kind);
        if (newToken)
        {
            TRACE(_T("DoAddToken() : Found token (parent)."));
            // Special handling function implementation, see comments above
            if (isImpl && (kind & tkAnyFunction))
                newToken->m_Args = args;
        }
    }
This will create a new Token, not re-use the old one.



Another bug I see is that void TokenTree::RemoveFile(int fileIdx) called twice when a file is reparsed. One in GUI handler, one in thread pool's task (canparse = m_TokenTree->ReserveFileForParsing(filename, true) != 0;)

No time to dig further......

If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5362
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: Rev9537 introduce a bug which will two class labels in the symbol browser
« Reply #1 on: February 09, 2014, 05:16:34 am »
To solve this, I suggest: avoid the data duplication, which is:
Do not save ancestor information in the m_Args, since we can simply get those information from m_AncestorsString or m_DirectAncestors. Also, template information (template parameter) can be get from m_TemplateArgument.

Another hack is to disable the check of m_Args in the function TokenExists() for a class kind Token, since those are used to check/merge overload functions.
If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.

Offline Alpha

  • Developer
  • Lives here!
  • *****
  • Posts: 1513
Re: Rev9537 introduce a bug which will two class labels in the symbol browser
« Reply #2 on: February 09, 2014, 05:58:15 am »
In my commit:
Code
http://cb.biplab.in/websvn/comp.php?repname=codeblocks&compare[]=/@9536&compare[]=/@9537
I had sent this data to m_Args because, from tracing through the rest of the code, I found there were already special handlers checking for it (but were previously useless, as m_Args was always empty for tkClass tokens).

Do not save ancestor information in the m_Args, since we can simply get those information from m_AncestorsString or m_DirectAncestors. Also, template information (template parameter) can be get from m_TemplateArgument.
I agree that this is probably the 'correct' solution (though, it may be difficult to do, as I seem to remember m_DirectAncestors not holding the necessary information).

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5362
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: Rev9537 introduce a bug which will two class labels in the symbol browser
« Reply #3 on: February 09, 2014, 06:50:11 am »
In my commit:
Code
http://cb.biplab.in/websvn/comp.php?repname=codeblocks&compare[]=/@9536&compare[]=/@9537
I had sent this data to m_Args because, from tracing through the rest of the code, I found there were already special handlers checking for it (but were previously useless, as m_Args was always empty for tkClass tokens).
I see. Here is the patch to fix the bug, we can still use m_Args in class kind tokens.
Code
1d8e6ea1388b71b37ef900f3303cf07f1b91fc26
 src/plugins/codecompletion/parser/parserthread.cpp |  3 ++-
 src/plugins/codecompletion/parser/tokentree.cpp    | 12 +++++++++---
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/src/plugins/codecompletion/parser/parserthread.cpp b/src/plugins/codecompletion/parser/parserthread.cpp
index ef73481..e9630b3 100644
--- a/src/plugins/codecompletion/parser/parserthread.cpp
+++ b/src/plugins/codecompletion/parser/parserthread.cpp
@@ -1374,7 +1374,8 @@ Token* ParserThread::DoAddToken(TokenKind       kind,
     if (   newToken
         && (newToken->m_TemplateArgument == m_TemplateArgument)
         && (   kind & tkAnyFunction
-            || newToken->m_Args == args ) )
+            || newToken->m_Args == args
+            || kind & tkAnyContainer ) )
     {
         ; // nothing to do
     }
diff --git a/src/plugins/codecompletion/parser/tokentree.cpp b/src/plugins/codecompletion/parser/tokentree.cpp
index 4c303c4..e57747d 100644
--- a/src/plugins/codecompletion/parser/tokentree.cpp
+++ b/src/plugins/codecompletion/parser/tokentree.cpp
@@ -186,9 +186,11 @@ int TokenTree::TokenExists(const wxString& name, const wxString& baseArgs, int p
         if (!curToken)
             continue;
 
+        // for a container token, there args member variables are used to store other information
+        // so, don't compare args for tkAnyContainer
         if (   (curToken->m_ParentIndex == parent)
             && (curToken->m_TokenKind   == kind)
-            && (curToken->m_BaseArgs    == baseArgs) )
+            && (curToken->m_BaseArgs == baseArgs || kind & tkAnyContainer) )
         {
             return result;
         }
@@ -249,7 +251,11 @@ int TokenTree::TokenExists(const wxString& name, const wxString& baseArgs, const
         if (!curToken)
             continue;
 
-        if (curToken->m_TokenKind == kind && curToken->m_BaseArgs == baseArgs)
+        // for a container token, there args member variables are used to store other information
+        // so, don't compare args for tkAnyContainer
+        if (  curToken->m_TokenKind == kind
+            && (   curToken->m_BaseArgs == baseArgs
+                || kind & tkAnyContainer ))
         {
             for ( TokenIdxSet::const_iterator pIt = parents.begin();
                   pIt != parents.end(); ++pIt )
@@ -626,7 +632,7 @@ bool TokenTree::CheckChildRemove(const Token* token, int fileIdx)
         else
             return false;  // one child is belong to another file
     }
-    return true;           // no children should be reserved, so we can safely remov the token
+    return true;           // no children should be reserved, so we can safely remove the token
 }
 
 void TokenTree::RecalcFreeList()

But I'm not sure the patch above cause other bugs. ;)


Quote
Do not save ancestor information in the m_Args, since we can simply get those information from m_AncestorsString or m_DirectAncestors. Also, template information (template parameter) can be get from m_TemplateArgument.
I agree that this is probably the 'correct' solution (though, it may be difficult to do, as I seem to remember m_DirectAncestors not holding the necessary information).
What information is not hold in m_DirectAncestors? Can you be more specific?
If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5362
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: Rev9537 introduce a bug which will two class labels in the symbol browser
« Reply #4 on: February 09, 2014, 03:40:18 pm »
...
I see. Here is the patch to fix the bug, we can still use m_Args in class kind tokens.
...
But I'm not sure the patch above cause other bugs. ;)
I commit the patch to trunk now rev 9640, fell free to adjust. I don't want the next nightly build version have such bug. ;D
If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.