This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC 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 diagnostics/fortran] Handle two locations for the same diagnostic. Convert all gfc_warning_1 and gfc_notify_std_1 calls


On 15 May 2015 at 10:39, Dodji Seketeli <dodji@redhat.com> wrote:
> Manuel LÃpez-IbÃÃez <lopezibanez@gmail.com> writes:
>> -/* 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.

I'm following the convention that parameter names are uppercase in the
description of functions. I can also say "Parameter WHICH specifies"
to be more explicit.


>>  /* 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.

I have a very poor memory for such details ;), since we are using
'private:' already, does it really hurt to be explicit and use
'public:' here?

>>    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.

OK.

> OK to commit with the changes above.

Thanks. If you insist on the two first changes, I'll do them, I just
want to confirm those.

Cheers,

Manuel.


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