Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New print assertions #601

Merged
merged 1 commit into from
Nov 16, 2020
Merged

New print assertions #601

merged 1 commit into from
Nov 16, 2020

Conversation

varunagrawal
Copy link
Collaborator

This PR adds new assertion functions to be able to test print outputs in a clean, functional way, reducing copy-pasta.

With these assertions, we can test for consistent printing across platforms, to improve the overall GTSAM experience.

  • assert_stdout_equal checks for the output when using std::cout <<.
  • assert_print_equal checks the output when using a class' print function, e.g. pose.print().

@varunagrawal varunagrawal added enhancement Improvement to GTSAM feature New proposed feature labels Nov 15, 2020
@varunagrawal varunagrawal self-assigned this Nov 15, 2020
@varunagrawal
Copy link
Collaborator Author

I came up with this PR because I found myself reinventing the stringstream-cout-redirect wheel too many times. 🙂

@jlblancoc
Copy link
Member

@varunagrawal What about adding all print() methods an extra optional parameter of type std::ostream, defaulting to std::cout, and which could be set to a local std::sstringstream in these functions? That idea has been around in my head for a long time, but it would imply touching many (all!) print() methods...
Thoughts?

@varunagrawal
Copy link
Collaborator Author

I have been thinking about the same thing, but I believe the better approach would be to overload the << operator for all classes that support the print function. Then the use of a stringstream would be trivial.

I did contemplate capturing the output in a stringstream, but there were 2 problems I wasn't sure about:

  1. For different systems and compilers, it made sense to perform the redirection so that we were reproducing real printing conditions, rather than a pseudo-print. The last thing I wanted was the headache of users complaining about printing being bad.
  2. The print() function is what we use in the wrapper to print details of an object, so I wasn't sure how adding an optional parameter would affect the wrapper's operation. @ProfFan is the wrapper expert and I just have working knowledge of it. Given that Fan is pretty busy these days with SwiftFusion, I opted out of that extra pain.

@varunagrawal varunagrawal merged commit c5daeb5 into develop Nov 16, 2020
@varunagrawal varunagrawal deleted the feature/cout-tests branch November 16, 2020 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement to GTSAM feature New proposed feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants