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

Keith Seitz
2011-02-10T21:45:23Z

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