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

Offline Huki

  • Multiple posting newcomer
  • *
  • Posts: 95
Re: Several improvements to Code Completion plugin
« Reply #15 on: September 09, 2013, 11:09:54 pm »
Additional info: the old test simply takes a rect from selection start to selection end, so it gives incorrect results when selecting multiple lines. I've attached some screenshots below - the red rectangle shows the incorrectly detected selection block. It's something that bothered me when I was selecting text (to copy, etc) and noticed the tooltip appearing behavior was not consistent.
Understood and I was able to reproduce it. Also I was able to implement it in a simpler way:

Can you try if this snippet fulfils all your requirements?
Yes, your code does the job nicely.

Quote
One note:
Scintilla fixes should go to the Scintilla project first.
Ok, I agree.

Quote
Also I think your wxDynamicLib modification to (wx)Scintilla are too much and we cannot accept them (this is my opinion and others should speak, too, especially Morten as he is the maintainer of this component in C::B).
You'll have to find another way to do it. Probably consulting with Scintilla devs for a way to do it or to ask them to add some API that will suit you.
Avoiding wxDynamicLib in Scintilla should be easy, I'll update that part soon.

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 12348
    • Travis build status
Re: Several improvements to Code Completion plugin
« Reply #16 on: September 10, 2013, 10:31:12 am »
Yes, your code does the job nicely.
In svn...
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline Alpha

  • Developer
  • Lives here!
  • *****
  • Posts: 1513
Re: Several improvements to Code Completion plugin
« Reply #17 on: September 10, 2013, 03:48:37 pm »
Yes, your code does the job nicely.
In svn...
The following is more succinct (but perhaps less readable?), and has the same execution.
Code: [Select]
Index: src/sdk/cbplugin.cpp
===================================================================
--- src/sdk/cbplugin.cpp (revision 9299)
+++ src/sdk/cbplugin.cpp (working copy)
@@ -207,12 +207,10 @@
             int startPos = stc->GetSelectionStart();
             int endPos = stc->GetSelectionEnd();
             int mousePos = stc->PositionFromPointClose(mousePosition->x, mousePosition->y);
-            if (mousePos == wxSCI_INVALID_POSITION)
+            if (startPos > mousePos || endPos < mousePos)
                 return wxEmptyString;
-            else if (startPos <= mousePos && mousePos <= endPos)
+            else
                 return selected_text;
-            else
-                return wxEmptyString;
         }
         else
             return selected_text;

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 12348
    • Travis build status
Re: Several improvements to Code Completion plugin
« Reply #18 on: September 10, 2013, 04:14:31 pm »
Don't like... I prefer the check for wxSCI_INVALID_POSITION
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline Huki

  • Multiple posting newcomer
  • *
  • Posts: 95
Re: Several improvements to Code Completion plugin
« Reply #19 on: September 11, 2013, 09:09:28 pm »
I have updated the scintilla grey out patch, let me know what you think of this one.

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 12348
    • Travis build status
Re: Several improvements to Code Completion plugin
« Reply #20 on: September 11, 2013, 10:31:17 pm »
1. Using PluginManager and GetDynamicLibrary is still bad. Please checkout the work done here: https://github.com/alpha0010/codeblocks_sf/tree/cc_interface
   This branch tries to implement a CCManager and similar stuff. There is a topic in the forum and I think Ollydbg posted a link to it in a previous comment.
2. You still modify the lexer for C++, why don't you just find the defines for the file after parsing the includes and pass them to Scintilla, I think it is possible.
3. Using char* to store strings is highly discouraged.
4. I think this will be one of the harder to apply patches, because it does weird things in many subsystems. Probably you should concentrate on the simpler things getting in and then this one.
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5229
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: Several improvements to Code Completion plugin
« Reply #21 on: September 13, 2013, 04:32:02 pm »
I got an error when apply one patch:
Quote
$ patch <../patches/CodeBlocks_patch_huki_130906/cc_enum_values.patch -p0
patching file `src/plugins/codecompletion/parser/expression.cpp'
patching file `src/plugins/codecompletion/parser/expression.h'
patch: **** malformed patch at line 36: Index: src/plugins/codecompletion/parser
/parserthread.cpp

EDIT: problem solved, I found that an empty line is missing, that is mostly caused by manually editing/spliting the patch file
Code: [Select]
@@ -88,7 +88,7 @@
 
     PostfixVector m_PostfixExpression;
     InfixVector   m_InfixExpression;
-    bool          m_Result;
+    long          m_Result; //MOD(huki), return type was (bool)
     bool          m_Status;
 };
 
Index: src/plugins/codecompletion/parser/parserthread.cpp
Note that there should be an empty line before the line "Index:.....".



About other patches:
cc_build_target_handling.patch
If a build target is changed, reparse the whole project, I think it is not necessory. E.g. If we change the build target in Codeblocks.cbp, the whole project get reparsed? That's not necessory I think. If the user do want to a reparse, he can right click on the context menu of the project manager, and select "reparse this project" sub menu.

cc_includes_parsing.patch
I think this is OK, priority header parsing is not needed here. The only issue is that some standalone header files included in a project, but those include files were not used/included in any cpp files, so they will be skipped by the parser.

« Last Edit: September 13, 2013, 05:11:30 pm 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 oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 12348
    • Travis build status
Re: Several improvements to Code Completion plugin
« Reply #22 on: September 13, 2013, 04:37:24 pm »
About other patches:
cc_build_target_handling.patch
If a build target is changed, reparse the whole project, I think it is not necessory. E.g. If we change the build target in Codeblocks.cbp, the whole project get reparsed? That's not necessory I think. If the user do want to a reparse, he can right click on the context menu of the project manager, and select "reparse this project" sub menu.
ollydbg: The user should never have to click reparse, never, everything should be automatic. We even should remove it.

Also keep in mind that very few people do project management as we do. I'm sure most people use multiple projects+debug/release targets instead of the C::B's approach.
So if this patch helps for such scenarios we should consider it.
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5229
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: Several improvements to Code Completion plugin
« Reply #23 on: September 13, 2013, 05:20:25 pm »
About cc_enum_values.patch
I think it is OK. I see you want to use the expression solver to calculate the enum value, which means if you are going to parse the code:

Code: [Select]
enum A
{
x = 1+2;
}

Then, you will get the correct enum value of x. I think a simple test code is necessary, you can simply follow the file under: \src\plugins\codecompletion\testing, maybe the enumerator.cpp, 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 #24 on: September 13, 2013, 05:37:20 pm »
Hm...

Can you post thorough descriptions for the problems you try to solve with every patch?
Steps to reproduce will be useful, too.
About the CC part: I'm interested on the patch: cc_fix_tokenizer.patch, cc_fix_ctor_dtor.patch, thanks. Maybe other cc patches, but I'm not viewing them yet. ;)
« Last Edit: September 13, 2013, 05:39:03 pm 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 #25 on: September 14, 2013, 07:51:31 am »
1. Using PluginManager and GetDynamicLibrary is still bad. Please checkout the work done here: https://github.com/alpha0010/codeblocks_sf/tree/cc_interface
   This branch tries to implement a CCManager and similar stuff. There is a topic in the forum and I think Ollydbg posted a link to it in a previous comment.
My main concern was to safely pass the CC function pointer for Scintilla's use. Using PluginManager we ensure that each cbEditor can get hold of the CC library during its lifetime and prevents the plugin from being accidentally unloaded (eg, upon user uninstall). The good thing is that the CodeCompletion object along with the parsers and everything will get freed as the user expects, but as the library still remains loaded, it is safe for Scintilla to keep calling our eval function (which will simply report that CC isn't enabled). Once all the open cbEditors are closed the library will go away too.

I see about the CCManager work. It would definitely make it easier for the CB SDK to communicate with CC for features like these, still don't know about Scintilla. Perhaps working with PluginManager through CCManager would be cleaner rather than doing it directly from cbEditor like now.

Quote
2. You still modify the lexer for C++, why don't you just find the defines for the file after parsing the includes and pass them to Scintilla, I think it is possible.
3. Using char* to store strings is highly discouraged.
There is not only #ifdef style defines, we also need to handle defines with value, or defined to other defines, function-like macros. The Scintilla lexer's expression parser is also bare-bones... on the other hand CC's expression solver will give us consistent results. Letting Scintilla call a CC function to get the result seems like the best option before me.
What I could do is limit all Scintilla modifications to LexerCPP.cxx. Using Scintilla lexer's PrivateCall() support this should be possible. It will also eliminate the need for char*.

Quote
4. I think this will be one of the harder to apply patches, because it does weird things in many subsystems. Probably you should concentrate on the simpler things getting in and then this one.
I was going to suggest the same actually. I'll put this one on hold till we've dealt with the other patches.

Offline Huki

  • Multiple posting newcomer
  • *
  • Posts: 95
Re: Several improvements to Code Completion plugin
« Reply #26 on: September 14, 2013, 07:58:20 am »
Quote from: ollydbg
I got an error when apply one patch:
Oh, sorry about that. I took care to keep extra lines, must have missed this one.

About cc_enum_values.patch
I think it is OK. I see you want to use the expression solver to calculate the enum value, which means if you are going to parse the code:

Code: [Select]
enum A
{
x = 1+2;
}

Then, you will get the correct enum value of x. I think a simple test code is necessary, you can simply follow the file under: \src\plugins\codecompletion\testing, maybe the enumerator.cpp, thanks.
I'm sending a patch file for \src\plugins\codecompletion\testing\enumerator.cpp to show some supported cases.
Note that my current implementation doesn't check for correct parent, it just accepts any parent (I'm talking about enums defined inside a particular class or namespace). Maybe I'll try to finish this part soon.

Offline Huki

  • Multiple posting newcomer
  • *
  • Posts: 95
Re: Several improvements to Code Completion plugin
« Reply #27 on: September 14, 2013, 08:05:20 am »
Hm...

Can you post thorough descriptions for the problems you try to solve with every patch?
Steps to reproduce will be useful, too.
About the CC part: I'm interested on the patch: cc_fix_tokenizer.patch, cc_fix_ctor_dtor.patch, thanks. Maybe other cc patches, but I'm not viewing them yet. ;)
For cc_fix_tokenizer.patch I already explained in a previous post. I'm quoting it again for convenience:

cc_fix_tokenizer: In Tokenizer::ReadToEOL(), which is used by HandleDefines(),
Code: [Select]
   //FIX(huki) if (ch <= _T(' ') && (p == buffer || *(p - 1) == ch))
    if (ch <= _T(' ') && p > buffer && *(p - 1) == ch)
i.e., do not remove the leading space. This is to differentiate defines containing brackets, from legitimate macros. Eg,
Code: Text
  1. #define MYMACRO() ...
Code: Text
  1. #define MYDEFINE (A + B)

The fix in Tokenizer::ReadParantheses() support '==' (previously it was taken as two assignments, i.e., ' = = '.
Rest should be commented already in the patch.


I'll discuss the others later tonight.

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5229
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: Several improvements to Code Completion plugin
« Reply #28 on: September 16, 2013, 06:09:39 pm »
For cc_fix_tokenizer.patch I already explained in a previous post. I'm quoting it again for convenience:
Thanks.

Quote
cc_fix_tokenizer: In Tokenizer::ReadToEOL(), which is used by HandleDefines(),
Code: [Select]
    //FIX(huki) if (ch <= _T(' ') && (p == buffer || *(p - 1) == ch))
    if (ch <= _T(' ') && p > buffer && *(p - 1) == ch)
i.e., do not remove the leading space. This is to differentiate defines containing brackets, from legitimate macros. Eg,
Code: Text
  1. #define MYMACRO() ...
Code: Text
  1. #define MYDEFINE (A + B)
I understand your change of the ReadToEOL(), I see that the only usage/call of the ReadToEOL function is in HandleDefines(), so here we should read the first token after the "#define", after that, we should check if there is a "(" directly after the token, if yes, then this is a function like macro, if not, this is a variable like macro.
The parameter stripUnneeded means: remove comments and compression spaces(two or more spaces should become one space), I think you change is OK. The string returned from the ReadToEOL have two cases:
If the string is beginning of the space -> this is a variable like macro definition, otherwise, it is a function like macro definition.
I think I need to update the comments in either HandleDefines and ReadToEOL function now. :)

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 #29 on: September 17, 2013, 12:14:02 am »
I have updated the cc_enum_value patch:
- In case of enum assignment to previous enum, it now looks under the correct parent instead of doing a global search.
- If the expression cannot be evaluated, leave it blank instead of resetting to zero (further enums will also be left blank till the next successful assignment).

In case you've already applied my previous patch, note that I have removed my changes to tokentree.cpp / tokentree.h (in my previous patch I had added a new TokenTree::TokenExists() function).

I'm trying to get each patch committed one by one, let me know if this one is ok.
« Last Edit: September 17, 2013, 12:16:59 am by Huki »