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

Pretty constructors in java parse tree #2152

Closed
wants to merge 2 commits into from
Closed

Conversation

kroening
Copy link
Member

@kroening kroening commented May 3, 2018

No description provided.

@@ -67,7 +67,9 @@ class java_bytecode_parse_treet
bool is_public, is_protected, is_private, is_static, is_final;
annotationst annotations;

virtual void output(std::ostream &out) const = 0;
virtual void output(
const irep_idt &class_name,
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like something the methodt should store rather than something every user has to pass to output()?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this would be an antipattern; its not unusual to require context for output, say think of indentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is reasonable to pass this in, I also think it is reasonable for the method to know what class it belongs to FWIW.

Copy link
Contributor

@thk123 thk123 left a comment

Choose a reason for hiding this comment

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

Don't duplicate magic string "<init>"


bool is_constructor() const
{
return base_name=="<init>";
Copy link
Contributor

Choose a reason for hiding this comment

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

Nooo - this re-duplicates the check that <init> is the name of constructors that I've painstakingly removed once! Either leave as is of remove the is_constructor in java_bytecode_convert_method

Copy link
Member

Choose a reason for hiding this comment

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

I think that is_constructor in java_bytecode_convert_method can be entirely removed. I'd replace the call in java_bytecode_convert_method.cpp:381 to use this new method. The other two uses java_bytecode_convert_method.cpp:575 and 1327 can be replaced by type.get_is_constructor().

Copy link
Contributor

@thk123 thk123 left a comment

Choose a reason for hiding this comment

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

Aside from previous review this looks OK. It would be good to have a test demonstrating this new behaviour.

@@ -67,7 +67,9 @@ class java_bytecode_parse_treet
bool is_public, is_protected, is_private, is_static, is_final;
annotationst annotations;

virtual void output(std::ostream &out) const = 0;
virtual void output(
const irep_idt &class_name,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is reasonable to pass this in, I also think it is reasonable for the method to know what class it belongs to FWIW.

@TGWDB
Copy link
Contributor

TGWDB commented Feb 22, 2021

Closing as this appears to be superseded by #2160.

@TGWDB TGWDB closed this Feb 22, 2021
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.

5 participants