Re: [PATCH diagnostics/fortran] Handle two locations for the same diagnostic. Convert all gfc_warning_1 and gfc_notify_std_1 calls

[Dodji Seketeli <dodji@redhat.com>]

Manuel López-Ibáñez <lopezibanez@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