Re: [PATCH 19/30] Add new location description access interface

[Zoran Zaric <Zoran.Zaric@amd.com>]

>> Thank you Tom for the review.
>>
>> To be honest, I am not sure how to do this with virtual functions, 
>> considering that the information held by the different location 
>> descriptions categories are so very different.
>>
>> The only way how I can see this being implemented if I encapsulate all 
>> the specifics of dealing with the different location descriptions 
>> (reading, writing, conversions between them and the composite/implicit 
>> pointer infrastructure) inside of that class hierarchy and change the 
>> dwarf_entry class to have a bunch of virtual functions that only make 
>> sense for one location description category.
>>
>> The reason why I didn't implement it that way in the first place is 
>> because I was following the existing model of how the struct value 
>> handling was implemented, where instead of dynamic casting there is an 
>> enumeration check to figure out what is described by an object.
>>
>> Don't get me wrong, I am not really against that change, but it would 
>> require a bit serious rework of the implementation.
>>
>> So if you really feel that this is a deal breaker, I could give 
>> another crack at it in the next version.
>>
>> Zoran
>>
> 
> After rereading my response, I realized that I probably need to explain 
> my thought process a bit more.
> 
> In the original code there were large switch cases that dealt with the 
> evaluation result (aka location description) based on a result 
> enumeration. This was done in two places:
> 
> - at the evaluator caller side and
> - composite/implicit pointer callback functions.
> 
> I first moved those pieces to be at the expr.c side and then when I 
> switched to the new classes, I have replaced the big switch statement, 
> with the if/else statements that used the dynamic cast to cast to the 
> appropriate class instead to referring to the enumeration to know which 
> data members are valid or not.
> 
> So in a sense, the ugly code was already there. Not to mention that the 
> struct value infrastructure is based on the same model.
> 
> Originally, I had an enumeration that would be checked inside of the 
> dwarf_entry class and return an object of the correct class but that was 
> functionally the same as having the dynamic cast so people advised me to 
> just use the dynamic cast because it was already used in gdb.
> 
> So Tom, when you say that the type-case looks bad, do you mean just the 
> fact that I am using the dynamic cast or the problem is more general 
> because the dwarf_entry class is supposed to encapsulate all that logic?
> 
> For the dynamic cast issue, I can always switch back to the enumeration 
> methodology that I've described and is similar to what was there before.
> 
> In the case of a potentially better design of the dwarf_entry classes, I 
> couldn't agree more. I was trying to first mimic the previous mechanism 
> so that people would find it easier to understand and accept.
> 
> The proper encapsulation was planned for a later time, but to be frank 
> the struct value requires the same type of remake which is not a modest 
> task in either case.
> 
> Hope this makes more sense,
> Zoran

Hi Tom,

I've invested a lot of time during the last two weeks and managed to 
change the design to be more C++ based and now most of the dynamic 
casting has been replaced with virtual functions which made the code a 
lot more elegant.

Unfortunately, there are still a few places left that use the casting 
but those are limited to the evaluator parts that model the logic behind 
how some of the DWARF operations are defined in the standard, which 
don't really make sense to be a DWARF entry class virtual functions.

This will all make more sense when I submit the new version of my patch 
series in a few days.

Zoran