This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch 0/3] Re: [RFA] c++/11734 revisited (and c++/12273)


Hi, Jan, thank you for taking the time to look at this. Believe me when I say, I feel for ya!

On 03/13/2011 03:28 PM, Jan Kratochvil wrote:
Although I wrote `isalnum' it is a wrong character class, for example
isalnum ('_') == 0 but `_' is a valid identifier character.

I have a cleanup that I've been working on which changed all this, but you've figured out before I have submitted my cleanup. I have done exactly as you suggest.


All these cases should be written like p[6] both for the IMO better
readability and for making it less fragile against bugs like `(*p + 6)'.

Done.


There may be also "task" catching but that is used by Ada and it already
worked before without such exception so it is probably OK.

I looked in the manual and the various gdb command help strings, but I could not find this. Maybe I didn't check Ada-specific commands? In any case, this whole linespec thing seems overtly fragile to begin with. It is a very poor abstraction for what is essentially a language-dependent task (of identifying names).


But it is trivial enough to add to this function, for whatever that's worth.

It seems to me here could be sufficient instead of is_overloaded just:
   if (*p == '(')

Or do you have a countercase where it would not work?

Nope. That was one of the cleanups that I'd arrived at last week, too.


*(p + 5) == '\''
->
strchr (get_gdb_completer_quote_characters (), p[5]) != NULL

Done.


Underrun of the strings you reported as present even in pre-physname GDB so
I have just filed it as:
	decode_linespec_1 vagrind errors on ""
	http://sourceware.org/bugzilla/show_bug.cgi?id=12535

Indeed! I've changed this to check that ptr > start (where start is ptr at entry to the function).


+  if (current_language->la_language == language_cplus
+      || current_language->la_language == language_java)
+      p = keep_name_info (p);

Wrong indentation.

Fixed.


It seems to me here could be sufficient instead of is_overloaded just:
   if (strchr (copy, '(') != NULL)

Or do you have a countercase where it would not work?

No countercase. I agree that strchr should be sufficient here when comparing the variable copy instead of real_saved_arg. Thank you for pointing that out -- I missed that.


It is approved with these changes if you agree with them.  I do not expect
anyone else is going to futher review it.

I'll await one final "OK" from you before committing, just in case you seen anything additional on which you'd like to comment.


Thanks again for looking at this. FWIW, I've attached only the linespec patch which includes the requested changes.

Keith

Attachment: linespec.c.patch
Description: Text document


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]