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)


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


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