[PATCH 4/6] Split event_location into subclasses

Tom Tromey tom@tromey.com
Tue Jan 18 16:59:19 GMT 2022


>> /* An event location used to set a stop event in the inferior.
>> This structure is an amalgam of the various ways
>> to specify where a stop event should be set.  */

Andrew> I think this comment is now out of date.

I'll update it.

>> -  union
>> +  /* Cached string representation of this location.  This is used, e.g., to
>> +     save stop event locations to file.  Malloc'd.  */
>> +  char *as_string = nullptr;

Andrew> I wonder if you considered making these private, at least for type,
Andrew> and adding a read accessor?  For as_string making it protected would
Andrew> seem like a better choice, we still need write access from
Andrew> set_event_location_string, but that itself could become a member
Andrew> function, and get_address_string_location would currently seem to need
Andrew> an accessor, though I notice in a later patch you change
Andrew> get_address_string_location to call to_string.

Andrew> If you think these changes would be better done later then this LGTM.

I didn't really consider it, but yeah, it makes sense.
I'd rather do this as a follow-up, though.

Tom


More information about the Gdb-patches mailing list