Re: [waffle] [RFC 3/3] wflinfo.c: Add a --json flag that prints json

[Chad Versace]

On 12/31/2015 09:51 AM, Dylan Baker wrote:
> 
> 
> On Wed, Dec 30, 2015 at 4:32 PM, Chad Versace  > wrote:
> 
> On 12/27/2015 07:49 AM, Frank Henigman wrote:
> > On Wed, Dec 16, 2015 at 8:37 PM,   > wrote:
> >> From: Dylan Baker  >
> >>
> >> This adds some code to print a JSON formatted version of data provided
> >> by wflinfo.
> >>
> >> Signed-off-by: Dylan Baker  >
> >> ---
> >>  src/utils/wflinfo.c | 174 
> +++-
> 
> 
> 
> >> +static void
> >> +json_print_context_flags(void)
> >> +{
> >> +int flag_count = sizeof(flags) / sizeof(flags[0]);
> 
> This should be
> 
> const int flag_count = ARRAY_SIZE(flags);
> 
> Optionally, you could even remove the flag_count variable altogether
> and use ARRAY_SIZE(flags) everywhere. But it doesn't really matter.
> 
> 
> This was copied from the original print_context_flags function. Should I 
> chang ethat to ARRAY_SIZE as well (presumably in a separate patch)?

Yes. It would be nice if you cleaned up print_context_flags in a separate patch.

> >> +
> >> +printf("[\n");
> >> +for (int i = 0; i < flag_count; i++) {
> >> +if ((flags[i].flag & context_flags) != 0) {
> >> +printf("\"%s\"", flags[i].str);
> >> +context_flags = context_flags & ~flags[i].flag;
> >> +if (i != flag_count) {
> >> +printf(",\n");
> >> +}
> >> +}
> >> +}
> >> +for (int i = 0; context_flags != 0; context_flags >>= 1, i++) {
> >> +if ((context_flags & 1) != 0) {
> >> +printf(",\n");
> >> +printf("0x%x", 1 << i);
> >> +}
> >> +}
> >> +printf("]");
> 
> Like Frank said, it would be nice if the json code didn't duplicate so 
> much of the
> original code. But, because you're not confident yet in C (as you say in 
> the cover letter),
> I'll accept the duplication in this function. I'll offer to deduplicate 
> the code myself if the patches
> go upstream.
> 
> 
> I'm curious what the (or a) good approach would be to fix this. I
> tried making a similar implementaiton in python (which I'm more
> comfortable with) and still had a hard time coming up with a good way
> to share code.
> 
> Personally I find being thrown in the deep in the best way to learn
> to swim, so if you could point me in the right (or at least a good)
> direction I'd like to take a stab at it.

Honestly, I also don't see a clean way to deduplicate the code for the
variable-length fields. Perhaps careful use of template strings that the
loops fill out with sprintf.

I suggest you don't worry about deduplication in this patch series, and
we can revisit it after the patches land. It would be silly to stall the json
output due to duplication of a tiny chunk of code.


> > I think some key strings should be a bit more specific or nested, like
> > "opengl version:" or "opengl : { version :" instead of just "version"
> > in case we expand some day to include things like egl version.
> 
> I agree. I also think the non-OpenGL key strings ("platform" and "api")
> should be prefixed with "waffle", just as they are in the normal wflinfo
> output.
> 
> 
> I started changing this locally after Frank's comments. My plan was
> to nest waffle specific information inside a waffle dict, OpenGL
> (including ES) inside of an opengl dict, and then leave room for
> other information to be in it's own data structures (say glx or egl)
> 
> something like:
> {
>  "waffle": {
>   "platform": "gbm"
>  },
>  "gl": {
>   "version": 3.1,
>   "profile": "core"
>  }
> }
> 
> Does that seem reasonable?

That looks good to me.

___
waffle mailing list
waffle@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/waffle

Re: [waffle] [RFC 3/3] wflinfo.c: Add a --json flag that prints json

[Dylan Baker]

Okay. I'll send out a v2 shortly.

On Tue, Jan 5, 2016 at 9:24 AM, Chad Versace  wrote:

> On 12/31/2015 09:51 AM, Dylan Baker wrote:
> >
> >
> > On Wed, Dec 30, 2015 at 4:32 PM, Chad Versace  > wrote:
> >
> > On 12/27/2015 07:49 AM, Frank Henigman wrote:
> > > On Wed, Dec 16, 2015 at 8:37 PM,   > wrote:
> > >> From: Dylan Baker >
> > >>
> > >> This adds some code to print a JSON formatted version of data
> provided
> > >> by wflinfo.
> > >>
> > >> Signed-off-by: Dylan Baker >
> > >> ---
> > >>  src/utils/wflinfo.c | 174
> +++-
> >
> >
> >
> > >> +static void
> > >> +json_print_context_flags(void)
> > >> +{
> > >> +int flag_count = sizeof(flags) / sizeof(flags[0]);
> >
> > This should be
> >
> > const int flag_count = ARRAY_SIZE(flags);
> >
> > Optionally, you could even remove the flag_count variable altogether
> > and use ARRAY_SIZE(flags) everywhere. But it doesn't really matter.
> >
> >
> > This was copied from the original print_context_flags function. Should I
> chang ethat to ARRAY_SIZE as well (presumably in a separate patch)?
>
> Yes. It would be nice if you cleaned up print_context_flags in a separate
> patch.
>
> > >> +
> > >> +printf("[\n");
> > >> +for (int i = 0; i < flag_count; i++) {
> > >> +if ((flags[i].flag & context_flags) != 0) {
> > >> +printf("\"%s\"", flags[i].str);
> > >> +context_flags = context_flags & ~flags[i].flag;
> > >> +if (i != flag_count) {
> > >> +printf(",\n");
> > >> +}
> > >> +}
> > >> +}
> > >> +for (int i = 0; context_flags != 0; context_flags >>= 1,
> i++) {
> > >> +if ((context_flags & 1) != 0) {
> > >> +printf(",\n");
> > >> +printf("0x%x", 1 << i);
> > >> +}
> > >> +}
> > >> +printf("]");
> >
> > Like Frank said, it would be nice if the json code didn't duplicate
> so much of the
> > original code. But, because you're not confident yet in C (as you
> say in the cover letter),
> > I'll accept the duplication in this function. I'll offer to
> deduplicate the code myself if the patches
> > go upstream.
> >
> >
> > I'm curious what the (or a) good approach would be to fix this. I
> > tried making a similar implementaiton in python (which I'm more
> > comfortable with) and still had a hard time coming up with a good way
> > to share code.
> >
> > Personally I find being thrown in the deep in the best way to learn
> > to swim, so if you could point me in the right (or at least a good)
> > direction I'd like to take a stab at it.
>
> Honestly, I also don't see a clean way to deduplicate the code for the
> variable-length fields. Perhaps careful use of template strings that the
> loops fill out with sprintf.
>
> I suggest you don't worry about deduplication in this patch series, and
> we can revisit it after the patches land. It would be silly to stall the
> json
> output due to duplication of a tiny chunk of code.
>
>
> > > I think some key strings should be a bit more specific or nested,
> like
> > > "opengl version:" or "opengl : { version :" instead of just
> "version"
> > > in case we expand some day to include things like egl version.
> >
> > I agree. I also think the non-OpenGL key strings ("platform" and
> "api")
> > should be prefixed with "waffle", just as they are in the normal
> wflinfo
> > output.
> >
> >
> > I started changing this locally after Frank's comments. My plan was
> > to nest waffle specific information inside a waffle dict, OpenGL
> > (including ES) inside of an opengl dict, and then leave room for
> > other information to be in it's own data structures (say glx or egl)
> >
> > something like:
> > {
> >  "waffle": {
> >   "platform": "gbm"
> >  },
> >  "gl": {
> >   "version": 3.1,
> >   "profile": "core"
> >  }
> > }
> >
> > Does that seem reasonable?
>
> That looks good to me.
>
>
___
waffle mailing list
waffle@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/waffle

[waffle] [PATCH v2 0/4] Add JSON interface to wflinfo

[baker . dylan . c]

From: Dylan Baker 

This is a second go at this series.

Notable changes from the first version:
- Style cleanups as suggested by Chad and Frank
- Restructuring the JSON output to have nested dictionaries for waffle
  and OpenGL specific information (this way it can be easily
  distinguished and easily extended with other information, like for the
  window system)
- Added a short -j option.

Dylan Baker (4):
  wflinfo.c: split out flags struct
  wflinfo.c: use ARRAY_SIZE macro
  wflinfo.c: split version, renderer, and vendor checks
  wflinfo.c: Add a --json flag that prints json

 src/utils/wflinfo.c | 260 +---
 1 file changed, 228 insertions(+), 32 deletions(-)

-- 
2.6.4

___
waffle mailing list
waffle@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/waffle