[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 <chad.versace at intel.com <mailto: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 <mailto:baker.dylan.c at gmail.com>> wrote:
>     >> From: Dylan Baker <baker.dylan.c at gmail.com <mailto: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 <mailto: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)?

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.