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

[Keith Seitz <keiths@redhat.com>]

I apologize to everyone, my Internet connectivity died on me last week, 
and I've only just had service restored this morning...

Honestly, I wasn't trying to ignore anyone. [Although I would love to be 
able to ignore linespec.c! :-)]

On 02/06/2011 02:45 PM, Jan Kratochvil wrote:

> I do not fully understand the reasons for part (b).  The old code is not nice
> but IMO neither is the new code (already checked in by the physname patch) due to
> the linespec.c caller.

One of the intents behind the patchset was to make the code behave the 
way it was documented. Forcing linespecs that would normally be handled 
by decode_compound into decode_variable based on the existence of 
overload information was (and still is) just plain wrong to me.

Mind you, that doesn't necessarily mean that decode_compound isn't evil 
or superfluous.

>  Moreover the new code has shown its regressions.

I'd like to make clear the meaning of the word "regression" in this 
context. Are we experiencing some fallout in linespecs because of this? 
Yes, we are. Are these bugs introduced by the patch? No. These are bugs 
that have existed for many years, but since the code did not do what the 
comments said it should do, these bugs lay hidden all this time inside 
decode_compound. And you have discovered another one (more on this 
later). And I found one more yesterday, too.

I did not set out to rewrite linespecs, just make them work with the 
least amount of invasion. I don't like rewriting tons of fragile, 
relatively undocumented code to fix a bug (even a non-trivial one) 
unless it is known/understood a priori that a redesign/rewrite is 
expected or wanted.

> If the code should be nice I tried archer-jankratochvil-linespec where
> linespec is based on the expressions.  Noted by Daniel Jacobowitz before:
> 	http://sourceware.org/ml/gdb-patches/2009-11/msg00266.html

I would love to see us allocate time to someone to finish the work on 
this approach. It would be a huge maintenance win -- even if it comes 
with its own set of problems or regressions. O:-) [That's the cost of 
progress IMO.]

> That is in general I would be either for futher not-nice fixing up the
> pre-physname code or for the expression way like archer-jankratochvil-linespec
> does.  Still at the moment your patchset gives the best user experience, just
> it is a new untested code which does not make it nice anyway.

I'm not entirely sure if I need to respond to your other messages on 
this topic -- I'm *way* out of context by now, but please ping me on IRC 
or email if there is something specific you'd like me to address.

Returning to the bug you discovered ("regression"), I do want to mention 
why decode_variable works and decode_compound does not. [Reminder: this 
is demonstrated by your namespace addition to the pr11734.exp test case.]

The answer is actually really simple: deocde_variable calls 
lookup_symbol with a valid block; decode_compound does not. Passing 
"get_selected_block (0)" to lookup_symbol [more or less] fixes the 
problems. [Follow-on patch to fix the fallout of that once I know what 
direction I'm being asked to go with this.]

I have expanded the original pr11734.exp test case even further by 
adding another namespace and class inside it, e.g., "A::B::MyClass", and 
this exposed another place where lookup_symbol was not getting a valid 
block to work with. Again, trivially fixed.

So, I'm not quite sure where I stand with this 11734/12273 (and other 
related bugs)... What do you want me to do?

Keith