Developer forums (C::B DEVELOPMENT STRICTLY!) > CodeCompletion redesign
Find Declaration of constructor doesn't work
ollydbg:
--- Quote from: oBFusCATed on January 26, 2014, 07:50:59 pm ---There is a translation that should apply with svn: http://cmpt.benbmp.org/codeblocks/patches/cc.ollydbg.svn.patch
--- End quote ---
Thanks for the conversion.
--- Quote ---ollydbg: your git patch is missing the email address and fails to apply cleanly with git am, please fix it.
--- End quote ---
I just manually delete the email address to anti-SPAM, ;). I will use a dummy email address next time.
Huki:
--- Quote from: MortenMacFly on March 02, 2014, 03:42:15 pm ---
--- Quote from: ollydbg on March 02, 2014, 03:06:02 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.
--- End quote ---
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.
--- End quote ---
I see ollydbg's patch is trying to handle both the "go to declaration" and "go to implementation" cases, so it sounds like a more complete solution (my patch only fixes the "go to impl" case).
@ollydbg:
But what does the AddConstructors() do? With my current patch I already get the "Multiple matches" dialog when using "go to impl" on the below code:
--- Code: ---cbEditor* cbedpp = new cbEditor(par, file, name, theme);
^^^^^^^^
--- End code ---
IMO we just need these 2 changes in CodeCompletion::OnGotoDeclaration:
1) Change:
--- Code: ---const bool isConstructor = CodeCompletionHelper::GetNextNonWhitespaceChar(editor->GetControl(), endPos) == _T('(')
&& CodeCompletionHelper::GetLastNonWhitespaceChar(editor->GetControl(), startPos) == _T(':');
--- End code ---
to:
--- Code: ---const bool isConstructor = CodeCompletionHelper::GetNextNonWhitespaceChar(editor->GetControl(), endPos) == _T('(');
--- End code ---
2) Make sure we haven't erased all results:
--- Code: --- // in a special case that a class definition don't have a constructor defined (implicitly defined by compiler)
// add shouldn't remove those class type tokens, so we need to restore the saved result
if (!result.size())
result = savedResult;
--- End code ---
Your patch has addressed both these points (and some cleanup for isClassOrConstructor, isConstructor, etc), so it's ok for me. I'm just not sure if the AddConstructors() part is required. Let me know what you think.
Huki:
--- Quote from: ollydbg on January 26, 2014, 04:08:10 am ---Let's still look at the sample code in this Re: Find Declaration of constructor doesn't work
What is the expect behavior?
--- Code: ---#include <iostream>
using namespace std;
class AAA //-----------------------(a)
{
public:
AAA(){}; //-----------------------(b)
int a;
};
int main()
{
AAA * p = new AAA();
^^^----------------------------------------------(1)
^^^-------------------------------(2)
cout << "Hello world!" << endl;
return 0;
}
--- End code ---
So, here is my rules:
case 1: find declaration in (1) should go to (a), not (b)
case 2: find declaration in (2) should go to (b), not (a)
But
case 3: do you think that find declaration in (1) should go to show a list box showing both (a) and (b), and let the user select where to go?
--- End quote ---
Ok, I think your AddConstructors() code was trying to achieve "case 3". My opinion is that only case 1 and 2 are required (i.e., find decl in (1) should only go to (a), the struct name).
So I suggest committing only the changes in CodeCompletion::OnGotoDeclaration of your patch, if you have no objections.
--- Quote from: ollydbg on January 26, 2014, 04:08:10 am ---I think Alpha has did some work which is quite similar to the solution of this issue, see the SVN commit WebSVN - codeblocks - Rev 9560 - /, but that's only for call-tip.
Question again here, does call-tip need to show a class definition Token? I basically think that a call-tip should show only a function prototype tip, unless this class does not have a constructor.
--- End quote ---
In fact I have some improvements for the calltips (typedef and macro functions), I'll test Alpha's patch when I'm going to submit those.
EDIT: BTW, I already get calltips for code like this:
--- Code: ---wxStaticText* static = new wxStaticText(|
--- End code ---
I think Alpha's patch was trying to get calltips for:
--- Code: ---wxStaticText static(|
--- End code ---
Though I'm not sure if it can work, because the variable "static" in the above code won't be parsed unless semi-colon is typed, right?
ollydbg:
Hi, Huki, sorry for the late reply, I'm busy those days. Let me explain something after I re-read the patch post months ago.
First, the function AddConstructors() is not going to handle the case 3, it is going to handle when user is not explicit define a constructor, such as
--- Code: ---class BBB
{
public:
int a;
};
--- End code ---
Now, we have some code like:
--- Code: ---BBB * p = new BBB(6);
--- End code ---
Now, user hover on the "BBB(6)", then generally, a search TokenTree will happen.
But note that we use the keyword "BBB", also, we search under the scope (global namespace and main() function body), the search result contains only one Token, which is the "class Token BBB". But what a user want is "go to function declaration of BBB()", so there is not such kind of Token in the result. So, as a fallback, we go to the class definition Token (class Token BBB).
Another case is:
--- Code: ---class AAA
{
public:
AAA(int b){};
int a;
};
int main()
{
AAA * p = new AAA(6);
--- End code ---
When user hover on "AAA(6)", we also do a search by keyword "AAA" in our Token (under the global scope and main() function body), then still you get one result "class Token AAA". Note here the constructor AAA is not in the result, because the constructor is a child Token of the class Token AAA. As a general rule, if an identifier is followed by a '(', this gives a hint that user need to go to some function like Tokens. So, I add the function AddConstructors(), which in the last stage of ResolveExpression(). So that both "class Token AAA" and "constructor AAA" is returned.
Later, "class Token AAA" is removed from the result, so we go to the correct declaration of the constructor.
I'm not sure my patch is going to handle "go to implementation xxxxx", I just think it solve some "go to declaration xxxx" issue.
Huki:
--- Quote from: ollydbg on March 06, 2014, 01:52:50 am ---Hi, Huki, sorry for the late reply, I'm busy those days.
--- End quote ---
No problem.
--- Quote from: ollydbg on March 06, 2014, 01:52:50 am ---
--- Code: ---class AAA
{
public:
AAA(int b){};
int a;
};
int main()
{
AAA * p = new AAA(6);
--- End code ---
When user hover on "AAA(6)", we also do a search by keyword "AAA" in our Token (under the global scope and main() function body), then still you get one result "class Token AAA". Note here the constructor AAA is not in the result, because the constructor is a child Token of the class Token AAA. As a general rule, if an identifier is followed by a '(', this gives a hint that user need to go to some function like Tokens. So, I add the function AddConstructors(), which in the last stage of ResolveExpression(). So that both "class Token AAA" and "constructor AAA" is returned.
Later, "class Token AAA" is removed from the result, so we go to the correct declaration of the constructor.
--- End quote ---
I now understand. I was confused about the sentence in bold because in my initial tests, the constructors were actually added to the result even without your AddConstructors() patch (and I got the "Multiple matches" dialog). But that was only because I was already inside the class scope when performing the "go to impl". In other cases (as in your example code) the AddConstructors() is indeed required.
--- Quote from: ollydbg on March 06, 2014, 01:52:50 am ---I'm not sure my patch is going to handle "go to implementation xxxxx", I just think it solve some "go to declaration xxxx" issue.
--- End quote ---
I tested your patch and can confirm it handles both cases as expected.
So I'm ok about committing your entire patch. :)
Navigation
[0] Message Index
[#] Next page
[*] Previous page
Go to full version