Author Topic: Several improvements to Code Completion plugin  (Read 117181 times)

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5229
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: Several improvements to Code Completion plugin
« Reply #60 on: February 08, 2014, 10:23:45 am »
Any news on my cc_enum_values.patch? I'm attaching it again and the supported test cases in cc_enum_test.patch.
- Uses expression solver to calculate the enum value, expand macro assignments.
- Supports enum assignment to a previous enum (checks under the correct parent).
- If the expression cannot be evaluated (eg, unknown macro), leave it and the next enums blank and don't reset it to zero.

Thanks, patches won't be forgotten !  :) But time is limited.
Applied in my local repo and works quite well. (when testing, I also found an bug introduced in rev 9601).

One question remains, what does this means:
Code: [Select]
           //FIX: Moved above.
            /*if (peek==ParserConsts::colon)
            {
                // bit specifier (eg, xxx:1)
                // -> walk to , or }
                SkipToOneOfChars(ParserConsts::commaclbrace);
            }*/
You are going to comment out this?
From my point of view, it should be removed, right? Because it was already handled before (in a if condition)
Code: [Select]
       if (peek==ParserConsts::colon)
        {
            peek = SkipToOneOfChars(ParserConsts::equals + ParserConsts::commaclbrace);
        }


About the include file expansion patch:
Hi, been busy for a while...
Today, I have tested the patch: cc_includes_parsing.patch
But I found it cause some GUI hang problem, see my report question about running parser in thread, I guess the reason is:

If you recursively parse the #include directive.

Code: [Select]
               if (!locked)
                    CC_LOCKER_TRACK_TT_MTX_LOCK(s_TokenTreeMutex)
                //this is a recursive call to Parse() function????
                result = thread->Parse();
                delete thread;

                if (!locked)
                    CC_LOCKER_TRACK_TT_MTX_UNLOCK(s_TokenTreeMutex)
So, you hold the s_TokenTreeMutex for a very long time. Some GUI function may try to query information from Tokentree, thus they get hangs.
I have a improvement patch to handle this (see attachment). The parser only parses cpp files in the project, recursive call the parse function when it meet the #include directive, the result is quite good.

Yes, my cc_includes_parsing patch did cause the UI to hang-up a bit because of the recursive parsing. I just tried your patch which frees up the mutex for a while before recursing, and the result is indeed quite good. Thanks!

I'm not sure the change of my improvement patch was good enough, I just add a small sleep function, any comments about such hack??


Thanks again for your contribution.




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: 5229
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: Several improvements to Code Completion plugin
« Reply #61 on: February 09, 2014, 04:03:03 pm »
Any news on my cc_enum_values.patch? I'm attaching it again and the supported test cases in cc_enum_test.patch.
- Uses expression solver to calculate the enum value, expand macro assignments.
- Supports enum assignment to a previous enum (checks under the correct parent).
- If the expression cannot be evaluated (eg, unknown macro), leave it and the next enums blank and don't reset it to zero.

Thanks, patches won't be forgotten !  :) But time is limited.
Applied in my local repo and works quite well. (when testing, I also found an bug introduced in rev 9601).

One question remains, what does this means:
Code: [Select]
           //FIX: Moved above.
            /*if (peek==ParserConsts::colon)
            {
                // bit specifier (eg, xxx:1)
                // -> walk to , or }
                SkipToOneOfChars(ParserConsts::commaclbrace);
            }*/
You are going to comment out this?
From my point of view, it should be removed, right? Because it was already handled before (in a if condition)
Code: [Select]
       if (peek==ParserConsts::colon)
        {
            peek = SkipToOneOfChars(ParserConsts::equals + ParserConsts::commaclbrace);
        }
I think the answer is YES, so I commit the patch to trunk now rev 9642, thanks!
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: 5229
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: Several improvements to Code Completion plugin
« Reply #62 on: February 12, 2014, 03:40:34 pm »
@Huki, some regression was reported, see Re: The 08 February 2014 build (9639) is out., so I revert rev 9369, I'm not sure which cause this hang issue, because our CC's code is a little mass, sorry about that. If you have time, you can help to clean up the code, thanks!
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 Huki

  • Multiple posting newcomer
  • *
  • Posts: 95
Re: Several improvements to Code Completion plugin
« Reply #63 on: February 15, 2014, 03:50:49 am »
About the include file expansion patch:
Hi, been busy for a while...
Today, I have tested the patch: cc_includes_parsing.patch
But I found it cause some GUI hang problem, see my report question about running parser in thread, I guess the reason is:

If you recursively parse the #include directive.

Code: [Select]
               if (!locked)
                    CC_LOCKER_TRACK_TT_MTX_LOCK(s_TokenTreeMutex)
                //this is a recursive call to Parse() function????
                result = thread->Parse();
                delete thread;

                if (!locked)
                    CC_LOCKER_TRACK_TT_MTX_UNLOCK(s_TokenTreeMutex)
So, you hold the s_TokenTreeMutex for a very long time. Some GUI function may try to query information from Tokentree, thus they get hangs.
I have a improvement patch to handle this (see attachment). The parser only parses cpp files in the project, recursive call the parse function when it meet the #include directive, the result is quite good.

Yes, my cc_includes_parsing patch did cause the UI to hang-up a bit because of the recursive parsing. I just tried your patch which frees up the mutex for a while before recursing, and the result is indeed quite good. Thanks!

I'm not sure the change of my improvement patch was good enough, I just add a small sleep function, any comments about such hack??

It's actually working very well - I have no more hangs now during batch parsing. I don't have a problem with the use of sleep function, it achieves the purpose, i.e., release the mutex before each include file, (very similar to the old behavior before the includes patch when the files were simply added to the end of the queue). The proper solution would probably be to ensure that the tokentree mutex is not accessed by the GUI while parsing, but this is not very easy.. lot of things the user does - mouse hover over a symbol, clicking into a function block, etc., will trigger the GUI to try access the mutex.. those are the times when I noticed the hangs actually.
In short, I'm happy with the current solution. :)
« Last Edit: February 15, 2014, 03:57:59 am by Huki »

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5229
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: Several improvements to Code Completion plugin
« Reply #64 on: February 16, 2014, 03:00:30 pm »
...
...
It's actually working very well - I have no more hangs now during batch parsing. I don't have a problem with the use of sleep function, it achieves the purpose, i.e., release the mutex before each include file, (very similar to the old behavior before the includes patch when the files were simply added to the end of the queue).
Thanks, yes, it works well (on my winXP system), so if no objections, I will commit this change.

Quote
The proper solution would probably be to ensure that the tokentree mutex is not accessed by the GUI while parsing, but this is not very easy.. lot of things the user does - mouse hover over a symbol, clicking into a function block, etc., will trigger the GUI to try access the mutex.. those are the times when I noticed the hangs actually.
The old behavior, when the parser is working, then gui access to tokentree was forbidden until parser finishes batch parsing, user will see some tips like "Parser is running, so no tip available". And later Loaden (one of C::B dev, but he was not active for years now) added many lockers to let the tree access from multiply thread.

So, do you think we need to change back to the old behavior?  I hate those lockers. :)

Quote
In short, I'm happy with the current solution. :)
Sure, me to, we can get more accurate tokens.

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: 5229
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: Several improvements to Code Completion plugin
« Reply #65 on: February 16, 2014, 03:07:47 pm »
...
...
It's actually working very well - I have no more hangs now during batch parsing. I don't have a problem with the use of sleep function, it achieves the purpose, i.e., release the mutex before each include file, (very similar to the old behavior before the includes patch when the files were simply added to the end of the queue).
Thanks, yes, it works well (on my winXP system), so if no objections, I will commit this change.


BTW: any one can test the patch(in this reply) on Linux? Thanks.
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 Huki

  • Multiple posting newcomer
  • *
  • Posts: 95
Re: Several improvements to Code Completion plugin
« Reply #66 on: February 25, 2014, 07:44:50 am »
...
...
It's actually working very well - I have no more hangs now during batch parsing. I don't have a problem with the use of sleep function, it achieves the purpose, i.e., release the mutex before each include file, (very similar to the old behavior before the includes patch when the files were simply added to the end of the queue).
Thanks, yes, it works well (on my winXP system), so if no objections, I will commit this change.
I'm ok with committing this patch, can confirm it works fine on Win 7 / 8.

Quote
The proper solution would probably be to ensure that the tokentree mutex is not accessed by the GUI while parsing, but this is not very easy.. lot of things the user does - mouse hover over a symbol, clicking into a function block, etc., will trigger the GUI to try access the mutex.. those are the times when I noticed the hangs actually.
The old behavior, when the parser is working, then gui access to tokentree was forbidden until parser finishes batch parsing, user will see some tips like "Parser is running, so no tip available". And later Loaden (one of C::B dev, but he was not active for years now) added many lockers to let the tree access from multiply thread.

So, do you think we need to change back to the old behavior?  I hate those lockers. :)
We can try changing back to the old behavior, I don't see any reason to access the tokentree before parsing is finished (we don't display tooltips anyway). Is it this kind of check that we find in NativeParser::MarkItemsByAI()?
Code: [Select]
    if (!m_Parser->Done())
    {
        wxString msg(_("The Parser is still parsing files."));
        msg += m_Parser->NotDoneReason();
        CCLogger::Get()->DebugLog(msg);
        return 0;
    }


Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5229
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: Several improvements to Code Completion plugin
« Reply #67 on: February 25, 2014, 07:49:21 am »
...
...
It's actually working very well - I have no more hangs now during batch parsing. I don't have a problem with the use of sleep function, it achieves the purpose, i.e., release the mutex before each include file, (very similar to the old behavior before the includes patch when the files were simply added to the end of the queue).
Thanks, yes, it works well (on my winXP system), so if no objections, I will commit this change.
I'm ok with committing this patch, can confirm it works fine on Win 7 / 8.
Thanks for the test.

Quote
Quote
The proper solution would probably be to ensure that the tokentree mutex is not accessed by the GUI while parsing, but this is not very easy.. lot of things the user does - mouse hover over a symbol, clicking into a function block, etc., will trigger the GUI to try access the mutex.. those are the times when I noticed the hangs actually.
The old behavior, when the parser is working, then gui access to tokentree was forbidden until parser finishes batch parsing, user will see some tips like "Parser is running, so no tip available". And later Loaden (one of C::B dev, but he was not active for years now) added many lockers to let the tree access from multiply thread.

So, do you think we need to change back to the old behavior?  I hate those lockers. :)
We can try changing back to the old behavior, I don't see any reason to access the tokentree before parsing is finished (we don't display tooltips anyway). Is it this kind of check that we find in NativeParser::MarkItemsByAI()?
Code: [Select]
    if (!m_Parser->Done())
    {
        wxString msg(_("The Parser is still parsing files."));
        msg += m_Parser->NotDoneReason();
        CCLogger::Get()->DebugLog(msg);
        return 0;
    }


Yes, m_Parser->Done() is the condition check to see whether Parser is currently working(parsing).
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: 5229
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: Several improvements to Code Completion plugin
« Reply #68 on: February 28, 2014, 05:52:49 am »
I'm ok with committing this patch, can confirm it works fine on Win 7 / 8.
Thanks for the test.
In SVN trunk now(r9665), it also includes a lot of code clean up, thanks for contribution.
« Last Edit: February 28, 2014, 05:57:36 am by ollydbg »
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 Huki

  • Multiple posting newcomer
  • *
  • Posts: 95
Re: Several improvements to Code Completion plugin
« Reply #69 on: March 01, 2014, 07:24:47 pm »
I'm ok with committing this patch, can confirm it works fine on Win 7 / 8.
Thanks for the test.
In SVN trunk now(r9665), it also includes a lot of code clean up, thanks for contribution.
Thanks for the commit.. just one question about this change in Parser::OnAllThreadsDone()
Code: [Select]
Index: src/plugins/codecompletion/parser/parser.cpp
===================================================================
--- src/plugins/codecompletion/parser/parser.cpp (revision 9664)
+++ src/plugins/codecompletion/parser/parser.cpp (revision 9665)
@@ -931,9 +888,7 @@
 
     // Do next task
     if (   !m_PoolTask.empty()
-        || !m_BatchParseFiles.empty()
-        || !m_PriorityHeaders.empty()
-        || !m_PredefinedMacros.IsEmpty() )
+        || !m_BatchParseFiles.empty() )
     {
         TRACE(_T("Parser::OnAllThreadsDone(): Still some tasks left, starting m_BatchTimer."));
         m_BatchTimer.Start(ParserCommon::PARSER_BATCHPARSE_TIMER_RUN_IMMEDIATELY, wxTIMER_ONE_SHOT);

Was the "|| !m_PredefinedMacros.IsEmpty()" removed on purpose?

Offline Huki

  • Multiple posting newcomer
  • *
  • Posts: 95
Re: Several improvements to Code Completion plugin
« Reply #70 on: March 01, 2014, 07:45:17 pm »
Two short patches:

1) A fix for tilde operator: make sure if it's really a destructor function, or just a variable with the bitwise NOT operation.
Code: [Select]
Index: src/plugins/codecompletion/codecompletion.cpp
===================================================================
--- src/plugins/codecompletion/codecompletion.cpp (revision 9271)
+++ src/plugins/codecompletion/codecompletion.cpp (working copy)
@@ -249,8 +249,9 @@
                 if (!word.IsEmpty())
                 {
                     NameUnderCursor.Clear();
-                    if (GetLastNonWhitespaceChar(control, start) == _T('~'))
-                        NameUnderCursor << _T('~');
+                    //FIX(huki), don't prepend '~' to search term (could be bitwise operator)
+                    //if (GetLastNonWhitespaceChar(control, start) == _T('~'))
+                    //    NameUnderCursor << _T('~');
                     NameUnderCursor << word;
                     ReturnValue = true;
                     IsInclude = false;
@@ -2182,8 +2233,9 @@
     const int startPos = editor->GetControl()->WordStartPosition(pos, true);
     const int endPos   = editor->GetControl()->WordEndPosition(pos, true);
     wxString target;
+    bool isDestructor = false;  //NOTE(huki), don't prepend '~' in cases of bitwise operator: eg: flag &= ~bits; and use "go to decl" on 'bits'.
     if (CodeCompletionHelper::GetLastNonWhitespaceChar(editor->GetControl(), startPos) == _T('~'))
-        target << _T('~');
+        isDestructor = true; //target << _T('~');
     target << editor->GetControl()->GetTextRange(startPos, endPos);
     if (target.IsEmpty())
         return;
@@ -2201,7 +2253,7 @@
     CC_LOCKER_TRACK_TT_MTX_LOCK(s_TokenTreeMutex)
 
     // special handle destructor function
-    if (target[0] == _T('~'))
+    if (isDestructor)   //if (target[0] == _T('~'))
     {
         TokenIdxSet tmp = result;
         result.clear();
@@ -2211,7 +2263,7 @@
             const Token* token = tree->at(*it);
             if (token && token->m_TokenKind == tkClass)
             {
-                token = tree->at(tree->TokenExists(target, token->m_Index, tkDestructor));
+                token = tree->at(tree->TokenExists(_T("~") + target, token->m_Index, tkDestructor));
                 if (token)
                     result.insert(token->m_Index);
             }

2) "Go to implementation" fails on constructor declaration. Upon "go to impl", the old code assumes it's a constructor only if the ':' prefix is found. This means user can't go to the ctor implementation from the class declaration for example. i.e.,
Code: [Select]
class CodeCompletion
{
    [...]
   
    /** Constructor */
    CodeCompletion(); <-- Use "go to implementation" on this line

    [...]
}

Fix:
Code: [Select]
Index: src/plugins/codecompletion/codecompletion.cpp
===================================================================
--- src/plugins/codecompletion/codecompletion.cpp (revision 9271)
+++ src/plugins/codecompletion/codecompletion.cpp (working copy)
@@ -2232,8 +2284,9 @@
         }
         if (isClassOrConstructor)
         {
-            const bool isConstructor = CodeCompletionHelper::GetNextNonWhitespaceChar(editor->GetControl(), endPos)   == _T('(')
-                                    && CodeCompletionHelper::GetLastNonWhitespaceChar(editor->GetControl(), startPos) == _T(':');
+            //NOTE(huki), Assume ctor if isImpl (user may want to go to ctor implementation from the class declaration).
+            const bool isConstructor = CodeCompletionHelper::GetNextNonWhitespaceChar(editor->GetControl(), endPos) == _T('(')
+                                    && (isImpl || CodeCompletionHelper::GetLastNonWhitespaceChar(editor->GetControl(), startPos) == _T(':'));
             for (TokenIdxSet::const_iterator it = result.begin(); it != result.end();)
             {
                 const Token* token = tree->at(*it);

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5229
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: Several improvements to Code Completion plugin
« Reply #71 on: March 02, 2014, 02:40:13 am »
...
Thanks for the commit.. just one question about this change in Parser::OnAllThreadsDone()
Code: [Select]
Index: src/plugins/codecompletion/parser/parser.cpp
===================================================================
--- src/plugins/codecompletion/parser/parser.cpp (revision 9664)
+++ src/plugins/codecompletion/parser/parser.cpp (revision 9665)
@@ -931,9 +888,7 @@
 
     // Do next task
     if (   !m_PoolTask.empty()
-        || !m_BatchParseFiles.empty()
-        || !m_PriorityHeaders.empty()
-        || !m_PredefinedMacros.IsEmpty() )
+        || !m_BatchParseFiles.empty() )
     {
         TRACE(_T("Parser::OnAllThreadsDone(): Still some tasks left, starting m_BatchTimer."));
         m_BatchTimer.Start(ParserCommon::PARSER_BATCHPARSE_TIMER_RUN_IMMEDIATELY, wxTIMER_ONE_SHOT);

Was the "|| !m_PredefinedMacros.IsEmpty()" removed on purpose?

Oh, it is a bug, fixed in trunk now, thanks.
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 MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9592
Re: Several improvements to Code Completion plugin
« Reply #72 on: March 02, 2014, 07:30:49 am »
2) "Go to implementation" fails on constructor declaration. Upon "go to impl", the old code assumes it's a constructor only if the ':' prefix is found. This means user can't go to the ctor implementation from the class declaration for example. i.e.,
[...]
I think I recall a patch floating around that deals with this already...
@ollydbg: Was it yours? It introduced a special method to handle constructors ... somewhere ... ???
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: http://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: http://wiki.codeblocks.org/index.php?title=FAQ

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5229
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: Several improvements to Code Completion plugin
« Reply #73 on: March 02, 2014, 03:06:02 pm »
2) "Go to implementation" fails on constructor declaration. Upon "go to impl", the old code assumes it's a constructor only if the ':' prefix is found. This means user can't go to the ctor implementation from the class declaration for example. i.e.,
[...]
I think I recall a patch floating around that deals with this already...
@ollydbg: Was it yours? It introduced a special method to handle constructors ... somewhere ... ???
Yes, I have one, I just search our forum for several minutes, and finally found one: Re: Find Declaration of constructor doesn't work.
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 MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9592
Re: Several improvements to Code Completion plugin
« Reply #74 on: March 02, 2014, 03:42:15 pm »
Yes, I have one, I just search our forum for several minutes, and finally found one: Re: Find Declaration of constructor doesn't work.
Yes, thats the one I recall. Well - if Hukis patch provides the same results I would vote for that instead, because it looks a bit simpler.
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: http://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: http://wiki.codeblocks.org/index.php?title=FAQ