Skip to content

Conversation

kroening
Copy link
Collaborator

@kroening kroening commented Sep 21, 2018

  • Each commit message has a non-empty body, explaining why the change was made.
  • My contribution is formatted in line with CODING_STANDARD.md.
  • Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
  • Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
  • My commit message includes data points confirming performance improvements (if claimed).
  • My PR is restricted to a single feature or bugfix.
  • White-space or formatting changes outside the feature-related changed lines are in commits of their own.

@kroening
Copy link
Collaborator Author

This will be hard to test: The effect is only visible when output is not redirected.

@kroening kroening force-pushed the console-colors branch 2 times, most recently from 652589b to 0033340 Compare September 21, 2018 16:20
Copy link
Collaborator

@tautschnig tautschnig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Essentially items 3 and 7 of the PR checklist. Please don't assign others when those cannot (yet) be ticked as it really means that the focus when reviewing shifts from the meat of the work to those superficial bits.

#endif
}

std::string console_message_handlert::command(unsigned c) const
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing documentation. The name does not in an obvious way relate to it's functionality, and what values c should take is a mystery.

protected:
const bool always_flush;
bool is_a_tty;
bool use_SGR;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

return m;
}

std::string command(unsigned c) const
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs.

return std::string();
}

// return to default
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the "default"? Default of what? Also: not Doxygen.

return command(0);
}

// set red foreground color
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doxygen, please.

: uit::PLAIN,
cmdline.isset("xml-ui")
? uit::XML_UI
: cmdline.isset("json-ui") ? uit::JSON_UI : uit::PLAIN,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also spurious clang-format change.

: cmdline.get_value("timestamp") == "wall"
? timestampert::clockt::WALL_CLOCK
: timestampert::clockt::NONE
: timestampert::clockt::NONE)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-format has had fun. Manually reverted.

Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passed Diffblue compatibility checks (cbmc commit: 2c09f3b).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/85603358

Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passed Diffblue compatibility checks (cbmc commit: 8a40b36).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/85897347

}

/// return to default formatting
/// return to default formatting, as defined by clienta
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"clienta" is probably a typo, but I wouldn't understand what the "client" is either.
Nit-pick: Change is in the wrong commit (should just be part of the original commit introducing the documentation).

@tautschnig tautschnig removed their assignment Sep 25, 2018
This enables colorful formatting of any message printed through the messaget
API.  Server-side implementation is provided for Windows and Unix consoles.
Failed properties are red, passing ones are green, and the overall result is
in bold.
This mimicks what newer versions of gcc do.
the summary result is now bold
Copy link
Contributor

@allredj allredj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passed Diffblue compatibility checks (cbmc commit: d999a3e).
Build URL: https://travis-ci.com/diffblue/test-gen/builds/85985276

@tautschnig tautschnig merged commit 9bd3c93 into develop Sep 27, 2018
@tautschnig tautschnig deleted the console-colors branch September 27, 2018 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants