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

Add port comment node #38

Merged
merged 8 commits into from
Feb 5, 2020
Merged

Add port comment node #38

merged 8 commits into from
Feb 5, 2020

Conversation

leonardt
Copy link
Owner

@leonardt leonardt commented Jan 31, 2020

Allow attaching comment to expressions ports, needed to fix phanrahan/magma#543

@leonardt leonardt changed the title Add expression comment node Add expression/port comment node Jan 31, 2020
@leonardt
Copy link
Owner Author

Also needed for ports.

@leonardt leonardt changed the title Add expression/port comment node Add port comment node Jan 31, 2020
@leonardt
Copy link
Owner Author

Changed it to just support ports for now since that's what is being used.

@@ -439,6 +439,17 @@ class BlockComment : public StructuralStatement, public BehavioralStatement {
~BlockComment(){};
};

class PortComment : public AbstractPort {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Think the best way to do this is to have a templated class WithComment<T> that has a unique_ptr<T> node + std::string comment, then we get all the other nodes w/ comment for free. Then add a convenience function too.

template<typename T> class WithComment : public T {
  ...
  std::string comment;
  std::string get_comment() {
    return this->comment;
  }
}

template<typename T> std::unique_ptr<WithComment<T>> AddComment(std::unique_ptr<T> node, std::string comment) {
  return std::make_unique<WithComment<T>>(std::move(node), comment);
}

(could also avoid template<typename T> and just have it over the ABC for ast nodes.

If you don't want to do ^, I would at least suggest changing the name to PortWithComment.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Clever design pattern, I've implemented this, but it requires that Nodes provide a "copy constructor" (quotes because it doesn't really create a clone of the unique pointer, instead it consumes it and uses it to recreate a new version of itself). This is required for implementing the base class constructor call, so WithComment<T> has a constructor of the form WithComment(std::unique_ptr<T> node, ...) and needs to call T(std::move(node)), so T needs to define a constructor that accepts a std::unique_ptr<T> and basically just recreates itself with the contents. Minor detail, but it needs to be added to all the types to be complete, however I've only added Port support for now since that's what's actually needed to unblock the downstream coreir issue.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah I see, yeah because of the inheritance from T. Makes sense. Saved by the implementation being in the header 😁

@leonardt leonardt merged commit afa165a into master Feb 5, 2020
@leonardt leonardt deleted the expr-comment branch February 5, 2020 20:36
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.

inline=True + verilator_debug=True issue
2 participants