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

Override print methods #760

Merged
merged 2 commits into from
Apr 30, 2021
Merged

Override print methods #760

merged 2 commits into from
Apr 30, 2021

Conversation

varunagrawal
Copy link
Collaborator

  • Override print methods.
  • Update wrapper.

@varunagrawal varunagrawal added python Related to python wrapper refactor Refactoring of code to remove rot labels Apr 29, 2021
@varunagrawal varunagrawal self-assigned this Apr 29, 2021
Copy link
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

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

Nice maintenance PR! Don't merge until you addressed comments and checked CI for warnings?

const KeyFormatter& formatter = DefaultKeyFormatter) const {
Factor::print(s, formatter);
/// print
virtual void print(
Copy link
Member

Choose a reason for hiding this comment

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

no virtual if override

@@ -177,6 +177,13 @@ namespace gtsam {
*/
VectorValues backSubstituteTranspose(const VectorValues& gx) const;

/// print graph
virtual void print(
Copy link
Member

Choose a reason for hiding this comment

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

no virtual. I recommend checking the warnings in a clang CI run to be sure I did not miss anything.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great idea. And yes I'll get rid of the virtuals where there are overrides.

const KeyFormatter& formatter = DefaultKeyFormatter) const = 0;

/// print
virtual void print(
Copy link
Member

Choose a reason for hiding this comment

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

no virtual

@@ -70,8 +70,9 @@ class GTSAM_EXPORT NonlinearFactor: public Factor {
/// @{

/** print */
virtual void print(const std::string& s = "",
const KeyFormatter& keyFormatter = DefaultKeyFormatter) const;
virtual void print(
Copy link
Member

Choose a reason for hiding this comment

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

no virtual

@@ -63,6 +63,13 @@ namespace gtsam {
/** Check equality */
GTSAM_EXPORT bool equals(const This& bn, double tol = 1e-9) const;

/// print
GTSAM_EXPORT void print(
Copy link
Member

Choose a reason for hiding this comment

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

Why is this the only one with GTSAM_EXPORT? Should we have that everywhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good question. My intent was to be consistent (since equals in this case is qualified with GTSAM_EXPORT) but it would be good to have a good hard look in a subsequent PR? I know Windows people will have all sorts of issues due to the lack of GTSAM_EXPORT.

@varunagrawal
Copy link
Collaborator Author

I had to add a bunch of virtual destructors to satisfy the compiler's warnings, but all the warnings with Clang have been addressed. 🎉

@varunagrawal varunagrawal merged commit 2c5be4d into develop Apr 30, 2021
@varunagrawal varunagrawal deleted the feature/override-print branch April 30, 2021 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Related to python wrapper refactor Refactoring of code to remove rot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants