Skip to content

Rename convert to convert_with_precedence #916

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

Merged

Conversation

owen-mc-diffblue
Copy link
Contributor

It is bad style to have an interface method the same name as an
internal method with a different signature. Then on overriding one of
these methods the compiler only finds the overloaded one with the wrong
signature, doesn't look up the base class and fails.

We fix this problem by renaming the protected method. This simplifies
the code in all derived classes.

This is an improvement on #888 , as suggested by @peterschrammel .

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.

Looks good to me, except for the line breaks.

@@ -205,7 +205,8 @@ class expr2ct
std::string convert_code_array_set(const codet &src, unsigned indent);
std::string convert_code_array_copy(const codet &src, unsigned indent);

virtual std::string convert(const exprt &src, unsigned &precedence);
// NOLINTNEXTLINE(whitespace/line_length)
virtual std::string convert_with_precedence(const exprt &src, unsigned &precedence);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a line break instead of silencing the linter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -37,7 +37,8 @@ class expr2cppt:public expr2ct
}

protected:
std::string convert(const exprt &src, unsigned &precedence) override;
// NOLINTNEXTLINE(whitespace/line_length)
std::string convert_with_precedence(const exprt &src, unsigned &precedence) override;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please wrap the line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

protected:
virtual std::string convert(const exprt &src, unsigned &precedence);
// NOLINTNEXTLINE(whitespace/line_length)
virtual std::string convert_with_precedence(const exprt &src, unsigned &precedence);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please wrap the line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@owen-mc-diffblue owen-mc-diffblue force-pushed the cleanup/using-expr2ct-convert branch from 94a673a to cf19251 Compare May 15, 2017 08:19
It is bad style to have an interface method the same name as an
internal method with a different signature. Then on overriding one of
these methods the compiler only finds the overloaded one with the wrong
signature, doesn't look up the base class and fails.

We fix this problem by renaming the protected method. This simplifies
the code in all derived classes.
@owen-mc-diffblue owen-mc-diffblue force-pushed the cleanup/using-expr2ct-convert branch from cf19251 to 351e3a7 Compare May 15, 2017 08:20
@kroening kroening merged commit 947a75c into diffblue:master May 15, 2017
owen-mc-diffblue pushed a commit to owen-mc-diffblue/cbmc that referenced this pull request May 18, 2017
Now that PR diffblue#916 has been merged, we don't need to manually forward
convert(const exprt) and convert(const typet) calls to the base class.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants