Add feature: Go to definition#402
Conversation
0a4d76c to
eb6634c
Compare
|
Demo: Click on any function, variable or typedef and press F12 |
|
Also, I will add to SeerSourceBrowserWidget: |
|
Quite impressive! Let me go over your details. |
|
@epasveer What do you think about this MR? |
Apologies. I've been busy with the Holidays and New Years. I'll review this MR shortly. I hope you've had a good Holidays. |
My apologies. Christmas isn't typically celebrated in Vietnam, so I assumed you had forgotten about this PR 😂 |
It's all good. I'll be back at things next week 😃 |
|
Getting to this now. Sorry for the delay. |
|
I like adding this feature. My understanding is the "definition" can be a function or it can be a data type. Seer already has this ability via the "Functions" or "Types" tab in the "Source/Symbol" browser in the upper left corner. Your idea makes it much simpler, in a workflow way. Which is always good 😄 I'm a little concerned about introducing a mutex and introducing a work thread. It adds complexity. What I would have done here is:
This is basically what you have now in your idea. Below is another idea for the rest.
Like Note, the text you provide from the cursor position may result in multiple occurrences of symbols or types. Have to decide how to handle this. (Open all, open none, open first). The text may match a function and a type. Again, have to decide how to handle this. One thing to note, the I don't see the need of a mutex, unless I'm missing something. Also, I'm not clear on what you mean by "syncronizing". Anyway, give my idea some thought. Oh, in your code there was a table of keywords. What are they for? Thanks for you great ideas! |
|
Hi @epasveer |
|
Okay. I'll review it again when you're done. |
e5d47b0 to
4737044
Compare
|
@epasveer |
Just so we're on the same page, solution for what? |
For distinguishing between variable, typedef, and function identifiers: |
I'm not surprised about this. With regular gdb, one has to decide to use one of these commands up front. ie: the user knows which one to use. I don't think we can read the user's mind to know which one to use. Perhaps, call each one, and if they result in multiple results, so be it. Open a source viewer for each one. I don't think we can do much else. |
|
Hi @epasveer , sorry for delay |
Sure. I'll review it again, just to be sure ("main" has other new changes) and will merge it if okay. |
82222d8 to
0eb914f
Compare
|
Ready for review, please check |
|
@epasveer Your changes look good to me |
|
| will safeguard against nonsense values I'm not sure if prefixing a "^" and postfixing a "$" to the name (which is a regex) might help eliminate the noise. |
|
I tried with you suggestion and it seems to work I tried with add() and the |
|
Hi,
I think that's because the symbol for Full text here. So the regex should be something like: For now, just add the What I'm trying to do is eliminate the noise as much as possible early on. Asking the SeerSourceBrowserWidget if a file exists is an expensive process, especially when there are a large number of source files. So the less checks, the better. |
b668207 to
3fad139
Compare
|
Your idea works. |
|
There was a problem in my test. It seems the symbols for my functions don't show arguments like your test does. Probably a compile/link setting. So I made a change to look for both styles. Can you take the latest from this PR and try your test(s) again? |
Excuse me, which test cases did you run? Please show me how to reproduce it |
It was: Using F12 for the |
|
It's weird This is gdb logout: |
|
Hi, Okay, this tells me you got my latest fix. And it works for my case. Does it work for your test case(s)? |
|
It works with my test case. |
I'd like to but I don't think I can. Your tests require one method. My tests require the other. It likely boils down to how the source files are compiled and linked, perhaps even what compiler. We can't ask the users to do anything so I think we need to make both calls. |
|
I've made one more changes. Because we now use exact definitions with I've removed that check (and code) and it works for my tests. Can you try your test again. If it works, I'll pull this MR. |
Can you try your tests with this latest MR when you have time? |
| // Searching for: "^functionName$". | ||
| // | ||
|
|
||
| emit refreshFunctionList(_idFunctionDefinition, "^" + _gotoDefIdentifier + "$"); |
There was a problem hiding this comment.
I’ve tried your changes, but this signal returns nothing in my test case, so I think we can remove this line.
I've tried to removing this line, and everything works fine
I’m not sure which test case you used, but if this line works for that test case, then we could keep it
|
I see you removed _sourceBrowserWidget and findFileWithRegex. |
Yeah, I did remove it. I didn't think it was needed at this point. If, as you point out, it would help with debugging the Linux kernel, we can revisit it then. |
1 similar comment
Yeah, I did remove it. I didn't think it was needed at this point. If, as you point out, it would help with debugging the Linux kernel, we can revisit it then. |
It is needed for my test case. So we need to keep it. |
Okay. Cool. I'll pull this MR. |



This MR adds a “Go to Definition” feature, mimicking VS Code

In which, I will use 2 short cuts: "F12" and "Ctrl + left click"