<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Dec 30, 2015 at 4:32 PM, Chad Versace <span dir="ltr"><<a href="mailto:chad.versace@intel.com" target="_blank">chad.versace@intel.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On 12/27/2015 07:49 AM, Frank Henigman wrote:<br>
> On Wed, Dec 16, 2015 at 8:37 PM, <<a href="mailto:baker.dylan.c@gmail.com">baker.dylan.c@gmail.com</a>> wrote:<br>
>> From: Dylan Baker <<a href="mailto:baker.dylan.c@gmail.com">baker.dylan.c@gmail.com</a>><br>
>><br>
>> This adds some code to print a JSON formatted version of data provided<br>
>> by wflinfo.<br>
>><br>
>> Signed-off-by: Dylan Baker <<a href="mailto:dylanx.c.baker@intel.com">dylanx.c.baker@intel.com</a>><br>
>> ---<br>
>> src/utils/wflinfo.c | 174 +++++++++++++++++++++++++++++++++++++++++++++++++++-<br>
<br>
<br>
<br>
</span><span class="">>> +static void<br>
>> +json_print_context_flags(void)<br>
>> +{<br>
>> + int flag_count = sizeof(flags) / sizeof(flags[0]);<br>
<br>
</span>This should be<br>
<br>
const int flag_count = ARRAY_SIZE(flags);<br>
<br>
Optionally, you could even remove the flag_count variable altogether<br>
and use ARRAY_SIZE(flags) everywhere. But it doesn't really matter.<br></blockquote><div><br></div><div>This was copied from the original print_context_flags function. Should I chang ethat to ARRAY_SIZE as well (presumably in a separate patch)?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
>> + GLint context_flags = 0;<br>
>> +<br>
>> + printf(" \"context flags\": ");<br>
>> +<br>
>> + glGetIntegerv(GL_CONTEXT_FLAGS, &context_flags);<br>
>> + if (glGetError() != GL_NO_ERROR) {<br>
>> + printf("\"WFLINFO_GL_ERROR\"\n");<br>
>> + return;<br>
>> + }<br>
>> +<br>
>> + if (context_flags == 0) {<br>
>> + printf("\"0x0\"\n");<br>
>> + return;<br>
>> + }<br>
<br>
</span>The json type of the "context flags" value is sometimes a string, sometimes an array.<br>
It should always be the same type, an array. When there are no flags, I think it should<br>
be an empty array.<br>
<span class=""><br></span></blockquote><div><br></div><div>You're right. Piglit doesn't use that information, so I didn't double check it.<br><br></div><div>I've made that change locally<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
>> +<br>
>> + printf("[\n");<br>
>> + for (int i = 0; i < flag_count; i++) {<br>
>> + if ((flags[i].flag & context_flags) != 0) {<br>
>> + printf(" \"%s\"", flags[i].str);<br>
>> + context_flags = context_flags & ~flags[i].flag;<br>
>> + if (i != flag_count) {<br>
>> + printf(",\n");<br>
>> + }<br>
>> + }<br>
>> + }<br>
>> + for (int i = 0; context_flags != 0; context_flags >>= 1, i++) {<br>
>> + if ((context_flags & 1) != 0) {<br>
>> + printf(",\n");<br>
>> + printf(" 0x%x", 1 << i);<br>
>> + }<br>
>> + }<br>
>> + printf(" ]");<br>
<br>
</span>Like Frank said, it would be nice if the json code didn't duplicate so much of the<br>
original code. But, because you're not confident yet in C (as you say in the cover letter),<br>
I'll accept the duplication in this function. I'll offer to deduplicate the code myself if the patches<br>
go upstream.<br></blockquote><div><br></div><div>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.<br><br></div><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
>> +<br>
>> + return;<br>
<br>
In C, (at least in the C projects I've worked with), the dominant code style omits<br>
the final return in void functions. The return is implicit.<br></blockquote><div><br></div><div>Okay, I've dropped that.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
>> +}<br>
<br>
<br>
<br>
>> + const char * vendor = get_vendor();<br>
>> + const char * renderer = get_renderer();<br>
>> + const char * version_str = get_version();<br>
<br>
</span>In Waffle, there should be no trailing space after the '*'. In other words, the<br>
correct style is:<br>
<br>
const char *vendor = ...<br></blockquote><div><br></div><div>I've fixed that, and one other case of the same thing locally.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
<br>
<br>
> I think some key strings should be a bit more specific or nested, like<br>
> "opengl version:" or "opengl : { version :" instead of just "version"<br>
> in case we expand some day to include things like egl version.<br>
<br>
</span>I agree. I also think the non-OpenGL key strings ("platform" and "api")<br>
should be prefixed with "waffle", just as they are in the normal wflinfo<br>
output.<br>
<br>
</blockquote></div><br></div><div class="gmail_extra">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)<br><br></div><div class="gmail_extra">something like:<br>{<br></div><div class="gmail_extra"> "waffle": {<br></div><div class="gmail_extra"> "platform": "gbm"<br> },<br></div><div class="gmail_extra"> "gl": {<br></div><div class="gmail_extra"> "version": 3.1,<br></div><div class="gmail_extra"> "profile": "core"<br> }<br>}<br><br></div><div class="gmail_extra">Does that seem reasonable?<br><br></div><div class="gmail_extra">Dylan<br></div></div>