Re: [PATCH iproute2] iproute: make clang happy
On Mon, Aug 20, 2018 at 5:52 PM, Stephen Hemminger wrote: > On Mon, 20 Aug 2018 16:44:28 -0700 > Mahesh Bandewar (महेश बंडेवार) wrote: > >> On Mon, Aug 20, 2018 at 4:38 PM, Mahesh Bandewar (महेश बंडेवार) >> wrote: >> > On Mon, Aug 20, 2018 at 3:52 PM, Stephen Hemminger >> > wrote: >> >> On Mon, 20 Aug 2018 14:42:15 -0700 >> >> Mahesh Bandewar wrote: >> >> >> >>> diff --git a/tc/m_ematch.c b/tc/m_ematch.c >> >>> index ace4b3dd738b..a524b520b276 100644 >> >>> --- a/tc/m_ematch.c >> >>> +++ b/tc/m_ematch.c >> >>> @@ -277,6 +277,7 @@ static int flatten_tree(struct ematch *head, struct >> >>> ematch *tree) >> >>> return count; >> >>> } >> >>> >> >>> +__attribute__((format(printf, 5, 6))) >> >>> int em_parse_error(int err, struct bstr *args, struct bstr *carg, >> >>> struct ematch_util *e, char *fmt, ...) >> >> >> >> I think the printf attribute needs to go on the function prototype >> >> here: >> >> tc/m_ematch.h:extern int em_parse_error(int err, struct bstr *args, >> >> struct bstr *carg, >> >> >> > The attributes are attached to the definitions only and not prototype >> > declarations. Please see the definition/declaration for jsonw_printf() >> > in the same patch. >> I take that back. Seems like it's fine either way. > > The reason to put the attributes in the .h file is that then the compiler > can test for misuse in other files. For example if em_parse_error had > a bad format in em_u32.c, then the warning would not happen unless > the attribute was on the function prototype. > correct, will take care in v2
Re: [PATCH iproute2] iproute: make clang happy
On Mon, 20 Aug 2018 16:44:28 -0700 Mahesh Bandewar (महेश बंडेवार) wrote: > On Mon, Aug 20, 2018 at 4:38 PM, Mahesh Bandewar (महेश बंडेवार) > wrote: > > On Mon, Aug 20, 2018 at 3:52 PM, Stephen Hemminger > > wrote: > >> On Mon, 20 Aug 2018 14:42:15 -0700 > >> Mahesh Bandewar wrote: > >> > >>> diff --git a/tc/m_ematch.c b/tc/m_ematch.c > >>> index ace4b3dd738b..a524b520b276 100644 > >>> --- a/tc/m_ematch.c > >>> +++ b/tc/m_ematch.c > >>> @@ -277,6 +277,7 @@ static int flatten_tree(struct ematch *head, struct > >>> ematch *tree) > >>> return count; > >>> } > >>> > >>> +__attribute__((format(printf, 5, 6))) > >>> int em_parse_error(int err, struct bstr *args, struct bstr *carg, > >>> struct ematch_util *e, char *fmt, ...) > >> > >> I think the printf attribute needs to go on the function prototype > >> here: > >> tc/m_ematch.h:extern int em_parse_error(int err, struct bstr *args, struct > >> bstr *carg, > >> > > The attributes are attached to the definitions only and not prototype > > declarations. Please see the definition/declaration for jsonw_printf() > > in the same patch. > I take that back. Seems like it's fine either way. The reason to put the attributes in the .h file is that then the compiler can test for misuse in other files. For example if em_parse_error had a bad format in em_u32.c, then the warning would not happen unless the attribute was on the function prototype.
Re: [PATCH iproute2] iproute: make clang happy
On Mon, Aug 20, 2018 at 4:44 PM, Mahesh Bandewar (महेश बंडेवार) wrote: > On Mon, Aug 20, 2018 at 4:38 PM, Mahesh Bandewar (महेश बंडेवार) > wrote: >> On Mon, Aug 20, 2018 at 3:52 PM, Stephen Hemminger >> wrote: >>> On Mon, 20 Aug 2018 14:42:15 -0700 >>> Mahesh Bandewar wrote: >>> diff --git a/tc/m_ematch.c b/tc/m_ematch.c index ace4b3dd738b..a524b520b276 100644 --- a/tc/m_ematch.c +++ b/tc/m_ematch.c @@ -277,6 +277,7 @@ static int flatten_tree(struct ematch *head, struct ematch *tree) return count; } +__attribute__((format(printf, 5, 6))) int em_parse_error(int err, struct bstr *args, struct bstr *carg, struct ematch_util *e, char *fmt, ...) >>> >>> I think the printf attribute needs to go on the function prototype >>> here: >>> tc/m_ematch.h:extern int em_parse_error(int err, struct bstr *args, struct >>> bstr *carg, >>> >> The attributes are attached to the definitions only and not prototype >> declarations. Please see the definition/declaration for jsonw_printf() >> in the same patch. > I take that back. Seems like it's fine either way. >>> >>> PS: I need to take the extern of those function prototypes. I can take care of this in v2
Re: [PATCH iproute2] iproute: make clang happy
On Mon, Aug 20, 2018 at 3:50 PM, Stephen Hemminger
wrote:
> On Mon, 20 Aug 2018 14:42:15 -0700
> Mahesh Bandewar wrote:
>
>>
>> if (is_json_context()) {
>> + json_writer_t *jw;
>> +
>> open_json_object("bittiming");
>> print_int(PRINT_ANY, "bitrate", NULL, bt->bitrate);
>> - jsonw_float_field_fmt(get_json_writer(),
>> - "sample_point", "%.3f",
>> - (float) bt->sample_point /
>> 1000.);
>> + jw = get_json_writer();
>> + jsonw_name(jw, "sample_point");
>> + jsonw_printf(jw, "%.3f",
>> + (float) bt->sample_point / 1000);
>
> I think it would be better to get rid of the is_json_context() here in the
> CAN code
> and just use the print_json functions completely. Most of the other code is
> able to
> do that already.
Seems like this is not the only location and need to be taken care
every where in the CAN code. I'll leave it to some JSON / print expert
Re: [PATCH iproute2] iproute: make clang happy
On Mon, Aug 20, 2018 at 4:38 PM, Mahesh Bandewar (महेश बंडेवार) wrote: > On Mon, Aug 20, 2018 at 3:52 PM, Stephen Hemminger > wrote: >> On Mon, 20 Aug 2018 14:42:15 -0700 >> Mahesh Bandewar wrote: >> >>> diff --git a/tc/m_ematch.c b/tc/m_ematch.c >>> index ace4b3dd738b..a524b520b276 100644 >>> --- a/tc/m_ematch.c >>> +++ b/tc/m_ematch.c >>> @@ -277,6 +277,7 @@ static int flatten_tree(struct ematch *head, struct >>> ematch *tree) >>> return count; >>> } >>> >>> +__attribute__((format(printf, 5, 6))) >>> int em_parse_error(int err, struct bstr *args, struct bstr *carg, >>> struct ematch_util *e, char *fmt, ...) >> >> I think the printf attribute needs to go on the function prototype >> here: >> tc/m_ematch.h:extern int em_parse_error(int err, struct bstr *args, struct >> bstr *carg, >> > The attributes are attached to the definitions only and not prototype > declarations. Please see the definition/declaration for jsonw_printf() > in the same patch. I take that back. Seems like it's fine either way. >> >> PS: I need to take the extern of those function prototypes.
Re: [PATCH iproute2] iproute: make clang happy
On Mon, Aug 20, 2018 at 3:52 PM, Stephen Hemminger wrote: > On Mon, 20 Aug 2018 14:42:15 -0700 > Mahesh Bandewar wrote: > >> diff --git a/tc/m_ematch.c b/tc/m_ematch.c >> index ace4b3dd738b..a524b520b276 100644 >> --- a/tc/m_ematch.c >> +++ b/tc/m_ematch.c >> @@ -277,6 +277,7 @@ static int flatten_tree(struct ematch *head, struct >> ematch *tree) >> return count; >> } >> >> +__attribute__((format(printf, 5, 6))) >> int em_parse_error(int err, struct bstr *args, struct bstr *carg, >> struct ematch_util *e, char *fmt, ...) > > I think the printf attribute needs to go on the function prototype > here: > tc/m_ematch.h:extern int em_parse_error(int err, struct bstr *args, struct > bstr *carg, > The attributes are attached to the definitions only and not prototype declarations. Please see the definition/declaration for jsonw_printf() in the same patch. > > PS: I need to take the extern of those function prototypes.
Re: [PATCH iproute2] iproute: make clang happy
On Mon, 20 Aug 2018 14:42:15 -0700 Mahesh Bandewar wrote: > diff --git a/tc/m_ematch.c b/tc/m_ematch.c > index ace4b3dd738b..a524b520b276 100644 > --- a/tc/m_ematch.c > +++ b/tc/m_ematch.c > @@ -277,6 +277,7 @@ static int flatten_tree(struct ematch *head, struct > ematch *tree) > return count; > } > > +__attribute__((format(printf, 5, 6))) > int em_parse_error(int err, struct bstr *args, struct bstr *carg, > struct ematch_util *e, char *fmt, ...) I think the printf attribute needs to go on the function prototype here: tc/m_ematch.h:extern int em_parse_error(int err, struct bstr *args, struct bstr *carg, PS: I need to take the extern of those function prototypes.
Re: [PATCH iproute2] iproute: make clang happy
On Mon, 20 Aug 2018 14:42:15 -0700
Mahesh Bandewar wrote:
>
> if (is_json_context()) {
> + json_writer_t *jw;
> +
> open_json_object("bittiming");
> print_int(PRINT_ANY, "bitrate", NULL, bt->bitrate);
> - jsonw_float_field_fmt(get_json_writer(),
> - "sample_point", "%.3f",
> - (float) bt->sample_point / 1000.);
> + jw = get_json_writer();
> + jsonw_name(jw, "sample_point");
> + jsonw_printf(jw, "%.3f",
> + (float) bt->sample_point / 1000);
I think it would be better to get rid of the is_json_context() here in the CAN
code
and just use the print_json functions completely. Most of the other code is
able to
do that already.
