Mir

Code review comment for lp:~josharenson/mir/install_glmark2

Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

46 +find_program(GLMARK2_ES2_MIR_FOUND glmark2-es2-mir)
47 +
48 +if(NOT GLMARK2_ES2_MIR_FOUND)
49 + message("warning: glmark2-es2-mir not found, performance tests _may_ fail.")
50 +endif()

Since we are not running the program automatically when building, I am not sure how useful it is to print this message at build time.

77 +#include <stdlib.h>

We also need <stdio.h> for popen, getline etc (we shouldn't depend on other headers bringing it in).

Some style issues (see http://unity.ubuntu.com/mir/cppguide/index.html):

85 +class GLMark2Test : public ::testing::Test
86 +{
...

Mir uses 4 space indentations.

88 + enum ResultFileType {RAW, JSON};

Lowercase identifiers for enum values.

103 + while((read = getline(&line, &len, in)) != -1)
105 + if(file_type == RAW)
107 + else if(file_type == JSON)
109 + if(boost::regex_match(line, matches, re_glmark2_score))

Spaces between keyword (if,while) and opening parenthesis.

105 + if(file_type == RAW)
106 + glmark2_output << line;

Braces around statement, since the 'else' clause also has braces.

94 + const char *cmd
93 + FILE *in;
...

'*' goes with type in our C++ code and const goes after the type. So: "FILE* in" and "char const* cmd"

98 + std::ofstream glmark2_output;

No need to define all variables in the beginning of the code block if it is more convenient (or necessary for RAII purposes) not to do so. So, for example it is allowed to have "std::ofstream glmark2_output{output_filename};" after the ASSERT.

123 + glmark2_output.close();

No need to manually close the file, it's closed on std::ofstream destruction.

review: Needs Fixing

« Back to merge proposal