[waffle] [PATCH v2 4/4] wflinfo.c: Add a --json flag that prints json
Chad Versace
chad.versace at intel.com
Fri Jan 8 14:31:42 PST 2016
Dylan, please plain text messages to the mailing list. It makes
review easier for everyone.
If you're using Google Chrome and GMail, then I recommend the "Fixed
Width Text for Gmail" extension [1] in the Chrome App Store.
[1]: https://chrome.google.com/webstore/detail/fixed-width-text-for-gmai/gamfbnidfgeeahogblblgafgpdhdkmib?utm_source=chrome-app-launcher-info-dialog
On 01/06/2016 03:03 PM, Dylan Baker wrote:
>
> On Wed, Jan 6, 2016 at 2:06 PM, Emil Velikov <emil.l.velikov at gmail.com <mailto:emil.l.velikov at gmail.com>> wrote:
>
> Hi Dylan,
>
> Perhaps I'm not the best person to comment on the JSON side of things
> (esp since Frank sent related series a few hours ago), then again a
> few things, that might be interesting.
>
> On 5 January 2016 at 19:46, <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>>
> >
> > v2: - Add -j short options (Frank)
> > - Make context flags always an array (Chad)
> > - Remove explicit returns from void functions (Chad)
> > - Fix spaces around "*" in variable definitions
> > - Nest waffle specific information in a waffle hash and OpenGL
> > specific information in an OpenGL hash
> > - use ARRAY_SIZE macro
> > - rebase on previous changes
> > ---
> > src/utils/wflinfo.c | 178 +++++++++++++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 176 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/utils/wflinfo.c b/src/utils/wflinfo.c
> > index 8ee95c6..456f91b 100644
> > --- a/src/utils/wflinfo.c
> > +++ b/src/utils/wflinfo.c
> > @@ -85,6 +85,9 @@ static const char *usage_message =
> > " --debug-context\n"
> > " Create a debug context.\n"
> > "\n"
> > + " -j, --json\n"
> > + " Print a JSON formatted summary.\n"
> > + "\n"
> > " -h, --help\n"
> > " Print wflinfo usage information.\n"
> > "\n"
> > @@ -104,6 +107,7 @@ enum {
> > OPT_VERBOSE = 'v',
> > OPT_DEBUG_CONTEXT,
> > OPT_FORWARD_COMPATIBLE,
> > + OPT_JSON = 'j',
> > OPT_HELP = 'h',
> > };
> >
> > @@ -115,6 +119,7 @@ static const struct option get_opts[] = {
> > { .name = "verbose", .has_arg = no_argument, .val = OPT_VERBOSE },
> > { .name = "debug-context", .has_arg = no_argument, .val = OPT_DEBUG_CONTEXT },
> > { .name = "forward-compatible", .has_arg = no_argument, .val = OPT_FORWARD_COMPATIBLE },
> > + { .name = "json", .has_arg = no_argument, .val = OPT_JSON },
> > { .name = "help", .has_arg = no_argument, .val = OPT_HELP },
> > { 0 },
> > };
> > @@ -252,6 +257,8 @@ struct options {
> >
> > bool verbose;
> >
> > + bool json;
> > +
> > bool context_forward_compatible;
> > bool context_debug;
> >
> > @@ -337,7 +344,7 @@ parse_args(int argc, char *argv[], struct options *opts)
> > opterr = 0;
> >
> > while (loop_get_opt) {
> > - int opt = getopt_long(argc, argv, "a:hp:vV:", get_opts, NULL);
> > + int opt = getopt_long(argc, argv, "a:hp:vV:j", get_opts, NULL);
> > switch (opt) {
> > case -1:
> > loop_get_opt = false;
> > @@ -395,6 +402,9 @@ parse_args(int argc, char *argv[], struct options *opts)
> > case OPT_DEBUG_CONTEXT:
> > opts->context_debug = true;
> > break;
> > + case OPT_JSON:
> > + opts->json = true;
> > + break;
> > case OPT_HELP:
> > write_usage_and_exit(stdout, EXIT_SUCCESS);
> > break;
> > @@ -563,6 +573,165 @@ print_context_flags(void)
> > printf("\n");
> > }
> >
> > +static void
> > +json_print_context_flags(void)
> > +{
> > + const int flag_count = ARRAY_SIZE(context_flags);
> const unsigned flag_count ?
>
> > + GLint gl_context_flags = 0;
> > +
> > + printf(" \"context flags\": ");
> > +
> > + glGetIntegerv(GL_CONTEXT_FLAGS, &gl_context_flags);
> > + if (glGetError() != GL_NO_ERROR) {
> > + printf("\"WFLINFO_GL_ERROR\"\n");
> > + return;
> > + }
> > +
> > + if (gl_context_flags == 0) {
> > + printf("[]");
> Missing \n ?
>
>
> Nope, gets covered later. This is a little odd to look at because that \n gets added later, because of the comma/no-comma madness that JSON enforces.
>
>
> > + return;
> > + }
> > +
> > + printf("[\n");
> > + for (int i = 0; i < flag_count; i++) {
> unsigned i ?
>
>
> > + if ((context_flags[i].flag & gl_context_flags) != 0) {
> > + printf(" \"%s\"", context_flags[i].str);
> > + gl_context_flags = gl_context_flags & ~context_flags[i].flag;
> > + if (i != flag_count) {
> This always evaluates to true, doesn't it ?
>
>
> Doesn't it evaluate to true in every case but the last iteration?
Nope. Because of the loop conditions, `0 <= i < flag_count` always holds.
In C, the following two are equivalent:
void foo(void)
{
for (A; B; C) {
STATEMENT_LIST;
}
}
void foo(void)
{
A;
goto loop_test;
loop_body;
STATEMENT_LIST;
C;
loop_test;
if (B) {
goto loop_body;
}
}
>
>
>
> > + printf(",\n");
> > + }
> > + }
> > + }
> > + for (int i = 0; gl_context_flags != 0; gl_context_flags >>= 1, i++) {
> > + if ((gl_context_flags & 1) != 0) {
> > + printf(",\n");
> We'll get two consecutive ",\n" considering the one above ?
>
>
> nope, because you don't get ",\n" in the last iteration of the above loop.
Is this still true considering that `i != flag_count` always?
>
>
>
> > + printf(" 0x%x", 1 << i);
> ... and we're missing the \n for the last entry.
>
> Easier way might be to do "leading" \n's ?
>
> > + }
> > + }
> > + printf(" ]");
> > +}
> > +
> > +static void
> > +json_print_extensions(bool use_stringi)
> > +{
> > + // Print extensions in JSON format
> > + printf(" \"extensions\": [\n");
> > + if (use_stringi) {
> > + GLint count = 0, i;
> > + const char *ext;
> > +
> > + glGetIntegerv(GL_NUM_EXTENSIONS, &count);
> > + if (glGetError() != GL_NO_ERROR) {
> > + printf("\"WFLINFO_GL_ERROR\"");
> Intentional difference in tabulation between previous function, here
> and else statement ?
>
>
> nope, I'll fix that.
>
>
>
> > + } else {
> > + for (i = 0; i < count; i++) {
> > + ext = (const char *) glGetStringi(GL_EXTENSIONS, i);
> > + if (glGetError() != GL_NO_ERROR)
> > + ext = "WFLINFO_GL_ERROR";
> > + printf(" \"%s\"%s\n", ext, (i + 1) < count ? "," : "");
> > + }
> > + }
> > + } else {
> > + char *extensions = (char *) glGetString(GL_EXTENSIONS);
> > + if (glGetError() != GL_NO_ERROR) {
> > + printf(" \"WFLINFO_GL_ERROR\"");
> > + } else {
> > + char *splitter = strtok(extensions, " ");
> > +
> Note to self: do we care if a driver returns a single extension
> (and/or empty extensions string) ?
>
> > + // The JSON doesn't strictly need to be newline seperated, but
> > + // it makes it much easier to read. Split the lines and add commas correctly
> > + while(true) {
> > + printf(" \"%s\"", splitter);
> > + splitter = strtok(NULL, " ");
> > +
> > + if (splitter != NULL) {
> > + printf(",\n");
> > + } else {
> > + break;
> > + }
> > + }
> > + }
> > + printf("\n");
> > + }
> > + printf(" ]\n");
> > +}
> > +
> > +/// @brief Print JSON formatted OpenGL (ES) information
> > +static bool
> > +print_json(const struct options *opts)
> > +{
> > + while(glGetError() != GL_NO_ERROR) {
> > + /* Clear all errors */
> Space between while and (... then again why the while loop ?
Yep. Space please.
> > + }
> > +
> > + const char *vendor = get_vendor();
> > + const char *renderer = get_renderer();
> > + const char *version_str = get_version();
> > +
> > + const char *platform = enum_map_to_str(platform_map, opts->platform);
> > + assert(platform != NULL);
> > +
> > + const char *api = enum_map_to_str(context_api_map, opts->context_api);
> > + assert(api != NULL);
> > +
> > + printf("{\n");
> > + printf(" \"waffle\": {\n");
> > + printf(" \"platform\": \"%s\",\n", platform);
> > + printf(" \"api\": \"%s\"\n", api);
> > + printf(" },\n");
> > + printf(" \"OpenGL\": {\n");
> > + printf(" \"vendor\": \"%s\",\n", vendor);
> > + printf(" \"renderer\": \"%s\",\n", renderer);
> > + printf(" \"version\": \"%s\"", version_str);
> > + // No comma is allowed on the last element. If this context doesn't have
> > + // context flags or verbose is not requested this will be the last element.
> > +
> > + int version = parse_version(version_str);
> > +
> > + if (opts->context_api == WAFFLE_CONTEXT_OPENGL && version >= 31) {
> > + // Add the comma and newline before the context flags.
> > + printf(",\n");
> > + json_print_context_flags();
> > + }
> > +
> > + // OpenGL and OpenGL ES >= 3.0 support glGetStringi(GL_EXTENSION, i).
> > + const bool use_getstringi = version >= 30;
> > +
> > + if (!glGetStringi && use_getstringi)
> > + error_get_gl_symbol("glGetStringi");
> > +
> > + if (opts->verbose) {
> > + // Add the command and newline
> > + printf(",\n");
> > +
> > + // See the equivalent section in print_wflinfo() for information about
> > + // this hunk
> > + const char *language_str = "None";
> > + if ((opts->context_api == WAFFLE_CONTEXT_OPENGL && version >= 20) ||
> > + opts->context_api == WAFFLE_CONTEXT_OPENGL_ES2 ||
> > + opts->context_api == WAFFLE_CONTEXT_OPENGL_ES3) {
> Align the last two ones - o (in opts) should be the same column as the (
>
>
> that looks like a readability nightmare:
>
> if ((opts->context_api == WAFFLE_CONTEXT_OPENGL && version >= 20) ||
> opts->context_api == WAFFLE_CONTEXT_OPENGL_ES2 ||
> opts->context_api == WAFFLE_CONTEXT_OPENGL_ES3) {
> language_str = (const char *) glGetString(GL_SHADING_LANGUAGE_VERSION);
> if (glGetError() != GL_NO_ERROR || language_str == NULL) {
> language_str = "WFLINFO_GL_ERROR";
> }
> }
That is indeed a readability nightmare. It's the bane of C projects that use 4 space indent.
I see three ways to fix it:
1. Use the indentation in your original patch. It's ugly, but it makes the indentation clear.
2. Do the below. It's not great, but it works.
if (long_blah1 ||
|| long_blah2
|| long_blah3) {
body;
}
3. Move the if-statements opening brace to its own line.
All the solutions are ugly, and the Waffle code is inconsistent regarding this problem.
So I don't care which you choose.
> > + language_str = (const char *) glGetString(GL_SHADING_LANGUAGE_VERSION);
> > + if (glGetError() != GL_NO_ERROR || language_str == NULL) {
> > + language_str = "WFLINFO_GL_ERROR";
> > + }
> > + }
> > +
> > + printf(" \"shading language version\": \"%s\",\n", language_str);
> > +
> > + json_print_extensions(use_getstringi);
> > +
> > + } else {
> > + // If verbose isn't being used, add a newline so that the closing curly
> > + // brace will be nicely formatted
> > + printf("\n");
> > + }
> > +
> > + printf(" }\n");
> > + printf("}\n");
> > +
> > + return true;
> Afaict you can make the function return void ?
>
> Seems that most (all?) of the comments here apply to the existing code
> in wflinfo.c where this was inspired/copied from. As mentioned by
> others - pondering on reusing the existing functions... I'll give it a
> quick test tomorrow.
>
> A bit tired so do take my comments with a grain of salt.
>
> That said (with or without the suggestions in 1 and 3) patches 1-3 are
> Reviewed-by: Emil Velikov <emil.velikov at collabora.com <mailto:emil.velikov at collabora.com>>
>
> -Emil
>
>
> I'll talk to Frank, but I think his series covers everything mine set out to do, if does, I'll just withdraw mine.
More information about the waffle
mailing list