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

[Dylan Baker]

On Wed, Dec 30, 2015 at 4:32 PM, Chad Versace <chad.versace at intel.com>
wrote:

> On 12/27/2015 07:49 AM, Frank Henigman wrote:
> > On Wed, Dec 16, 2015 at 8:37 PM,  <baker.dylan.c at gmail.com> wrote:
> >> From: Dylan Baker <baker.dylan.c at gmail.com>
> >>
> >> This adds some code to print a JSON formatted version of data provided
> >> by wflinfo.
> >>
> >> Signed-off-by: Dylan Baker <dylanx.c.baker at intel.com>
> >> ---
> >>  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)?


>
> >> +    GLint context_flags = 0;
> >> +
> >> +    printf("    \"context flags\": ");
> >> +
> >> +    glGetIntegerv(GL_CONTEXT_FLAGS, &context_flags);
> >> +    if (glGetError() != GL_NO_ERROR) {
> >> +        printf("\"WFLINFO_GL_ERROR\"\n");
> >> +        return;
> >> +    }
> >> +
> >> +    if (context_flags == 0) {
> >> +        printf("\"0x0\"\n");
> >> +        return;
> >> +    }
>
> The json type of the "context flags" value is sometimes a string,
> sometimes an array.
> It should always be the same type, an array. When there are no flags, I
> think it should
> be an empty array.
>
>
You're right. Piglit doesn't use that information, so I didn't double check
it.

I've made that change locally


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


>
> >> +
> >> +    return;
>
> In C, (at least in the C projects I've worked with), the dominant code
> style omits
> the final return in void functions. The return is implicit.
>

Okay, I've dropped that.


>
> >> +}
>
>
>
> >> +    const char * vendor =  get_vendor();
> >> +    const char * renderer = get_renderer();
> >> +    const char * version_str = get_version();
>
> In Waffle, there should be no trailing space after the '*'. In other
> words, the
> correct style is:
>
>     const char *vendor = ...
>

I've fixed that, and one other case of the same thing locally.


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

Dylan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/waffle/attachments/20151231/91fe9f7e/attachment.html>