Several improvements to Code Completion plugin

[ollydbg]

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

            //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

        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

                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.

[ollydbg]

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

            //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

        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!

[ollydbg]

@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!

[Huki]

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

                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.

[ollydbg]

...
...
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.

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.

In short, I'm happy with the current solution.

Sure, me to, we can get more accurate tokens.

[ollydbg]

...
...
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.

[Huki]

...
...
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.

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

    if (!m_Parser->Done())
    {
        wxString msg(_("The Parser is still parsing files."));
        msg += m_Parser->NotDoneReason();
        CCLogger::Get()->DebugLog(msg);
        return 0;
    }

[ollydbg]

...
...
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.

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

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

[ollydbg]

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.

[Huki]

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

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?

[Huki]

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

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

class CodeCompletion
{
    [...]
    
    /** Constructor */
    CodeCompletion(); <-- Use "go to implementation" on this line

    [...]
}

Fix:

Code

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

[ollydbg]

...
Thanks for the commit.. just one question about this change in Parser::OnAllThreadsDone()

Code

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.

[MortenMacFly]

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 ...

[ollydbg]

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.

[MortenMacFly]

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.