[v2,0/5] Tab complete convenience variables

  Implement tab completion for convenience variables and registers in
expressions. This has been something that annoyed me about gdb for
years.

Antonio Rische (5):
  gdb: Do not create variables when parsing expressions
  gdb: Tab complete internalvars in expressions
  gdb/testsuite: Test completion of convenience variables
  gdb: Tab-complete registers in expressions
  gdb/testsuite: Test completion of register names in expressions

 gdb/ada-exp.h                         | 12 ++++-
 gdb/ax-gdb.c                          | 53 ++++++++++++++++++----
 gdb/cli/cli-utils.c                   |  2 +-
 gdb/completer.c                       | 27 +++++++++++
 gdb/expop.h                           | 55 ++++++++++++++++++++++-
 gdb/parse.c                           |  7 ++-
 gdb/testsuite/gdb.base/completion.exp | 64 ++++++++++++++++++++++++---
 7 files changed, 199 insertions(+), 21 deletions(-)
  

On 8/27/24 10:49 PM, Antonio Rische wrote:
> Implement tab completion for convenience variables and registers in
> expressions. This has been something that annoyed me about gdb for
> years.
>
> Antonio Rische (5):
>    gdb: Do not create variables when parsing expressions
>    gdb: Tab complete internalvars in expressions
>    gdb/testsuite: Test completion of convenience variables
>    gdb: Tab-complete registers in expressions
>    gdb/testsuite: Test completion of register names in expressions
>
>   gdb/ada-exp.h                         | 12 ++++-
>   gdb/ax-gdb.c                          | 53 ++++++++++++++++++----
>   gdb/cli/cli-utils.c                   |  2 +-
>   gdb/completer.c                       | 27 +++++++++++
>   gdb/expop.h                           | 55 ++++++++++++++++++++++-
>   gdb/parse.c                           |  7 ++-
>   gdb/testsuite/gdb.base/completion.exp | 64 ++++++++++++++++++++++++---
>   7 files changed, 199 insertions(+), 21 deletions(-)
>
Hi!

Thanks for the contribution! This is definitely appreciated.

I don't understand the completion stuff that you worked on, so I can't 
review the code in depth, but nothing jumped out to me as an obvious 
problem. I have 2 small comments on how you chose to split the patches, 
but it's nothing major. Firstly, we tend to prefer we the test is added 
along with the feature, so it would be preferable if you merged patches 
2 and 3 together, and patches 4 and 5 together.

Second, and this is more of a question than a requirement, why did you 
choose to split internalvar completion and register completion in 2 
different patches? Since they both relate to completion of $-prefixed 
things, and both are relatively simple, I personally don't see much 
value in splitting, but I recognize I may just be missing something.

I have tested your patches in my fedora 40, and I can confirm that it 
does add completion support and doesn't add any regression, so feel free 
to add my tag in future versions that don't change the code 
significantly. Tested-By: Guinevere Larsen <blarsen@redhat.com>
  

Thanks for the testing!

I'll combine the testsuite changes with the commits adding
functionality. The only reason for separation of internalvars
and registers is to minimize commit size (and that I hadn't
looked into how to complete registers originally); there isn't
any technical reason it's necessary nor do I have a strong
preference there.

I'm perfectly happy making this a two-patch series. I do think
"gdb: Do not create variables when parsing expressions" should
be its own patch though, keeping internal refactoring separate
from observable changes in the feature set.

Antonio

On Friday, August 30th, 2024 at 6:44 PM, Guinevere Larsen <blarsen@redhat.com> wrote:

> On 8/27/24 10:49 PM, Antonio Rische wrote:
> 
> > Implement tab completion for convenience variables and registers in
> > expressions. This has been something that annoyed me about gdb for
> > years.
> > 
> > Antonio Rische (5):
> > gdb: Do not create variables when parsing expressions
> > gdb: Tab complete internalvars in expressions
> > gdb/testsuite: Test completion of convenience variables
> > gdb: Tab-complete registers in expressions
> > gdb/testsuite: Test completion of register names in expressions
> > 
> > gdb/ada-exp.h | 12 ++++-
> > gdb/ax-gdb.c | 53 ++++++++++++++++++----
> > gdb/cli/cli-utils.c | 2 +-
> > gdb/completer.c | 27 +++++++++++
> > gdb/expop.h | 55 ++++++++++++++++++++++-
> > gdb/parse.c | 7 ++-
> > gdb/testsuite/gdb.base/completion.exp | 64 ++++++++++++++++++++++++---
> > 7 files changed, 199 insertions(+), 21 deletions(-)
> 
> Hi!
> 
> Thanks for the contribution! This is definitely appreciated.
> 
> I don't understand the completion stuff that you worked on, so I can't
> review the code in depth, but nothing jumped out to me as an obvious
> problem. I have 2 small comments on how you chose to split the patches,
> but it's nothing major. Firstly, we tend to prefer we the test is added
> along with the feature, so it would be preferable if you merged patches
> 2 and 3 together, and patches 4 and 5 together.
> 
> Second, and this is more of a question than a requirement, why did you
> choose to split internalvar completion and register completion in 2
> different patches? Since they both relate to completion of $-prefixed
> things, and both are relatively simple, I personally don't see much
> value in splitting, but I recognize I may just be missing something.
> 
> I have tested your patches in my fedora 40, and I can confirm that it
> does add completion support and doesn't add any regression, so feel free
> to add my tag in future versions that don't change the code
> significantly. Tested-By: Guinevere Larsen blarsen@redhat.com
> 
> 
> --
> Cheers,
> Guinevere Larsen
> She/Her/Hers