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

Documentation to diplomacy.(LazyModule part) #2311

Merged
merged 55 commits into from
Jun 7, 2020

Conversation

sequencer
Copy link
Member

Related issue: #2308

Type of change: other enhancement

Impact: no functional change

Development Phase: proposal

Release Notes

This work is based on my previously code analysis to diplomacy in https://github.com/cnrv/diplomacy.
Since @richardxia decides to implement a scaladoced diplomacy, I create this PR based on my originaly analysis.

Originally inline comments is based on my first (actually third, first and second are totally fails) diving to diplomacy codebase, thus, it contains some misunderstanding and inaccuracy. In this PR, I will base on my previously work, writing a specific documentation to diplomacy LazyModule and Node.

Diplomacy must be a elegant design from mind of geniuses. However is not fully documentated, which established a protective screen between normal users to diplomaty based Module ecosystem. I hope detailed documentation like this can help us to break this screen, making diplomacy being used by everyone, not only SiFive and UCB.

This PR may contains 3 to 4 commits before finish first version, hope it can be reviewed by designers for a better quality.

@mwachs5
Copy link
Contributor

mwachs5 commented Feb 28, 2020

@richardxia do you want to pull this into your PR, since you have chipsalliance access and it might be easier to incrementally work on @sequencer 's PR?

@sequencer
Copy link
Member Author

Most Nodes documentation haven’t added yet. if you are not mind, I can work on these this weekend. But I’m still confused by resloveStar.

Copy link
Contributor

@mwachs5 mwachs5 left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! Lots of suggestions for clarity and to add my understanding a bit more. My requested changes are to not make any actual functional code changes in this PR

src/main/scala/diplomacy/LazyModule.scala Outdated Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Outdated Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Outdated Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Outdated Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Outdated Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Outdated Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Outdated Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Outdated Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Outdated Show resolved Hide resolved
@sequencer
Copy link
Member Author

@mwachs5 Thanks for your review! I mistakenly introduced some code change.
And some code change are added for better understanding to code. If not appropriate, I will revoke those changes.

@richardxia
Copy link
Member

@richardxia do you want to pull this into your PR, since you have chipsalliance access and it might be easier to incrementally work on @sequencer 's PR?

Sorry for the late reply; was on international flights at the end of last week. There are two reasons I'm a bit hesitant to do this:

  1. @sequencer himself will be unable to push to the branch, since he won't have access.
  2. I'm a bit worried about scope creep, since there's already a ton of draft quality text written. I'd rather narrow down the scope and get those pieces to 100% rather than have a little bit of 80% ready text everywhere.

Regarding 1), if @sequencer checks the "Allow edits from maintainers" checkbox on the bottom right of his PR, then anyone with commit access to chipsalliance/rocket-chip.git will be able to push to his branch, even if it exists on his fork.

Going to try to take a look at all the PRs/comments regarding Diplomacy docs, both in this PR and in mine now.

@mwachs5
Copy link
Contributor

mwachs5 commented Mar 2, 2020

Ah cool didn't know about that feature.

Copy link
Contributor

@mwachs5 mwachs5 left a comment

Choose a reason for hiding this comment

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

Some more comments, just getting started on Nodes.scala :)

src/main/scala/diplomacy/LazyModule.scala Outdated Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Outdated Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Outdated Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Outdated Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Outdated Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Outdated Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Outdated Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Outdated Show resolved Hide resolved
src/main/scala/diplomacy/Nodes.scala Outdated Show resolved Hide resolved
src/main/scala/diplomacy/Nodes.scala Outdated Show resolved Hide resolved
Copy link
Member

@richardxia richardxia left a comment

Choose a reason for hiding this comment

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

I am out of time at the moment to continue reviewing this, but I have left some detailed comments for at least part of this.

src/main/scala/diplomacy/LazyModule.scala Outdated Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Outdated Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Outdated Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Outdated Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Outdated Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Outdated Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Outdated Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Outdated Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Outdated Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Outdated Show resolved Hide resolved
Copy link
Contributor

@mwachs5 mwachs5 left a comment

Choose a reason for hiding this comment

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

Can we split the "Nodes.scala" PR separate from the other one? I think Nodes is super important, but shouldn't necessarily block the other.

src/main/scala/diplomacy/Nodes.scala Outdated Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Outdated Show resolved Hide resolved
@sequencer sequencer changed the title [WIP] Documentation to diplomacy. [WIP] Documentation to diplomacy.(LazyModule part) Mar 3, 2020
@sequencer
Copy link
Member Author

I refactor some code in 5ddd8eb, which refactor some codes for better understanding, if not appropriate, I'll refactor to 4713e5b.

@mwachs5 mwachs5 requested a review from richardxia May 23, 2020 20:43
Copy link
Member Author

@sequencer sequencer left a comment

Choose a reason for hiding this comment

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

@mwachs5 @richardxia I append a list of changed codes for clearer code reading experience, if not appropriate, I will revert these changes.

src/main/scala/diplomacy/LazyModule.scala Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Show resolved Hide resolved
src/main/scala/diplomacy/LazyModule.scala Show resolved Hide resolved
@mwachs5
Copy link
Contributor

mwachs5 commented May 30, 2020

@richardxia this is blocked on you requesting changes. Can you re-review?

Copy link
Member

@richardxia richardxia left a comment

Choose a reason for hiding this comment

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

I haven't been very active on this PR in the last few months, but I took another quick look, and it looks great! Thinks for adding much-needed documentation on this!

@mwachs5 mwachs5 merged commit e1c26df into chipsalliance:master Jun 7, 2020
This pull request was closed.
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.

5 participants