Manuel López-Ibáñez <lopeziba...@gmail.com> writes:
> Thanks for the review. I followed all your suggestions. For the
> accessor functions, I was not sure what type you would prefer, so I
> implemented them as C++ methods and made use of 'private' to be sure
> they are the only way to access the locations array. If you want me to
> change it, just tell me what you prefer. I also replaced
> diagnostic_same_locus with diagnostic_same_line.
Great, thank you.
>
> Bootstrapped & regression tested on x86_64-linux-gnu.
>
> OK?
Yes, but with the slight changes below. Sorry I spotted them just now,
but they are quick to fix, I believe.
>
> Manuel.
>
[...]
> -/* Expand the location of this diagnostic. Use this function for
> consistency. */
> +/* Return the location associated to this diagnostic. WHICH specifies
Here, I think only the 'W' (in WHICH) should be uppercase.
[...]
> /* The type of a text to be formatted according a format specification
> along with a list of things. */
> struct text_info
> {
> +public:
As this is a struct, the 'public' here is not necessary, as the members
are public by default.
> const char *format_spec;
> va_list *args_ptr;
> int err_no; /* for %m */
> - location_t *locus;
> void **x_data;
> +
> + inline location_t & set_location (unsigned int index_of_location)
I think it's less surprising to have the this function take two
parameters: The index_of_location and the new new location.
So that it's used by doing: set_location (0, the_new_location);
> + {
> + gcc_checking_assert (index_of_location < MAX_LOCATIONS_PER_MESSAGE);
> + return this->locations[index_of_location];
> + }
> +
> + inline location_t get_location (unsigned int index_of_location) const
> + {
> + gcc_checking_assert (index_of_location < MAX_LOCATIONS_PER_MESSAGE);
> + return this->locations[index_of_location];
> + }
> +
> +private:
> + location_t locations[MAX_LOCATIONS_PER_MESSAGE];
> };
>
[...]
OK to commit with the changes above.
Thanks a lot!
--
Dodji
