On Wed, Dec 30, 2015 at 4:32 PM, Chad Versace <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> wrote:
> >> From: Dylan Baker <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>
> >> ---
> >> 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
_______________________________________________
waffle mailing list
waffle@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/waffle
