Okay. I'll send out a v2 shortly. On Tue, Jan 5, 2016 at 9:24 AM, Chad Versace <chad.vers...@intel.com> wrote:
> On 12/31/2015 09:51 AM, Dylan Baker wrote: > > > > > > On Wed, Dec 30, 2015 at 4:32 PM, Chad Versace <chad.vers...@intel.com > <mailto:chad.vers...@intel.com>> wrote: > > > > On 12/27/2015 07:49 AM, Frank Henigman wrote: > > > On Wed, Dec 16, 2015 at 8:37 PM, <baker.dyla...@gmail.com > <mailto:baker.dyla...@gmail.com>> wrote: > > >> From: Dylan Baker <baker.dyla...@gmail.com <mailto: > baker.dyla...@gmail.com>> > > >> > > >> This adds some code to print a JSON formatted version of data > provided > > >> by wflinfo. > > >> > > >> Signed-off-by: Dylan Baker <dylanx.c.ba...@intel.com <mailto: > dylanx.c.ba...@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. > >
_______________________________________________ waffle mailing list waffle@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/waffle
