[v2] Do not classify C struct members as a filename

  There is existing logic in C/C++ expression parsing to avoid classifying
names as a filename when they are a field on the this object. This
change extends this logic to also avoid classifying names after a
struct-op (-> or .) as a filename, which otherwise causes a syntax
error.

Thus, it is now possible in the file

    #include <map>
    struct D {
        void map();
    }
    D d;

to call

    (gdb) print d.map()

where previously this would have been a syntax error.

Tested on gdb.cp/*.exp

gdb/ChangeLog:

        * c-exp.y (lex_one_token, classify_name, yylex): Don't classify
        names after a structop as a filename

gdb/testsuite/ChangeLog:

        * gdb.cp/filename.cc, gdb.cp/filename.exp: Test that member
        functions with the same name as an include file are parsed
        correctly.
---

Removed an accidental unrelated change in varobj.c

 gdb/ChangeLog                     |  5 +++++
 gdb/c-exp.y                       | 24 +++++++++++++-----------
 gdb/testsuite/ChangeLog           |  6 ++++++
 gdb/testsuite/gdb.cp/filename.cc  | 15 ++++++++++++++-
 gdb/testsuite/gdb.cp/filename.exp | 10 ++++++++--
 5 files changed, 46 insertions(+), 14 deletions(-)
  

On 2018-01-24 11:51, Leszek Swirski via gdb-patches wrote:
> There is existing logic in C/C++ expression parsing to avoid 
> classifying
> names as a filename when they are a field on the this object. This
> change extends this logic to also avoid classifying names after a
> struct-op (-> or .) as a filename, which otherwise causes a syntax
> error.
> 
> Thus, it is now possible in the file
> 
>     #include <map>
>     struct D {
>         void map();
>     }
>     D d;
> 
> to call
> 
>     (gdb) print d.map()
> 
> where previously this would have been a syntax error.
> 
> Tested on gdb.cp/*.exp

Hi Leszek,

I was able to reproduce the problem and confirmed that the patch fixes 
it.  I'm not very strong in the lexing/parsing area, but the change in 
c-exp.y makes sense to me.  I'd like to give others a chance to comment, 
so let's wait a few days.  If nobody answer, we can merge it.  One nit, 
since we are now using C++, can you use bool instead of int?  It won't 
match the old surrounding code, but that's a good reason to modernize 
the existing code later :).

Do you know if we have a test for the same thing, but with the "this" 
pointer (which was already worked before this patch)?  If not, it would 
be nice to add it to the test as well.  You could add a call to 
D::includefile and continue/break there.  From there, you could test 
calling this->includefile().

Simon
  

>
> I was able to reproduce the problem and confirmed that the patch fixes
> it.  I'm not very strong in the lexing/parsing area, but the change in
> c-exp.y makes sense to me.  I'd like to give others a chance to comment, so
> let's wait a few days.  If nobody answer, we can merge it.  One nit, since
> we are now using C++, can you use bool instead of int?  It won't match the
> old surrounding code, but that's a good reason to modernize the existing
> code later :).
>
> Do you know if we have a test for the same thing, but with the "this"
> pointer (which was already worked before this patch)?  If not, it would be
> nice to add it to the test as well.  You could add a call to D::includefile
> and continue/break there.  From there, you could test calling
> this->includefile().


Hi Simon, thanks for taking a look.

Sure, I can update the tests (I'll add some tests for checking
"c.includefile" while I'm at it) and change the int to a bool. I'm not 100%
sure on the procedure here, do you want me to upload a v3 as a new thread?

- Leszek
  

On 2018-01-25 05:03, Leszek Swirski via gdb-patches wrote:
> Hi Simon, thanks for taking a look.
> 
> Sure, I can update the tests (I'll add some tests for checking
> "c.includefile" while I'm at it) and change the int to a bool. I'm not 
> 100%
> sure on the procedure here, do you want me to upload a v3 as a new 
> thread?
> 
> - Leszek

Hi Leszek,

Yes, sending a v3 would be ideal.  It's usually a new thread (as in 
there is no "Re:"), but some people like to use git-send-email's 
--in-reply-to option to refer to the previous version, so that the 
different versions thread nicely in email clients.  It's up to you.

Simon
  

>>>>> "Leszek" == Leszek Swirski via gdb-patches <gdb-patches@sourceware.org> writes:

Leszek> There is existing logic in C/C++ expression parsing to avoid classifying
Leszek> names as a filename when they are a field on the this object. This
Leszek> change extends this logic to also avoid classifying names after a
Leszek> struct-op (-> or .) as a filename, which otherwise causes a syntax
Leszek> error.

Nice, thanks for doing this.

Leszek> -	if (parse_completion && tokentab2[i].token == ARROW)
Leszek> +	if (tokentab2[i].token == ARROW)

Leszek> -	  if (parse_completion)
Leszek> -	    last_was_structop = 1;
Leszek> +	  last_was_structop = 1;

This change makes sense (and thanks for updating that comment as well),
but I wonder whether this changes the behavior in some case.  Elsewhere
in lex_one_token there is a check of saw_structop:

      else if (saw_structop)
	return COMPLETE;

Previously this return could only be taken if parse_completion was set,
but now I think it could be taken in other situations.

So, I suspect "parse_completion &&" should be stuck in there.

Tom
  

> This change makes sense (and thanks for updating that comment as well),
> but I wonder whether this changes the behavior in some case.  Elsewhere
> in lex_one_token there is a check of saw_structop:
>
>       else if (saw_structop)
>         return COMPLETE;
>
> Previously this return could only be taken if parse_completion was set,
> but now I think it could be taken in other situations.
>
> So, I suspect "parse_completion &&" should be stuck in there.

Nice catch, thanks! I looked at the other users of saw_structop and
last_was_structop, and it looks like this is the only location that
needs an update. PTAL at v4.