search

lowRISC / style-guides

lowRISC Style Guides
Creative Commons Attribution 4.0 International
359 stars 122 forks source link

Clarification changes #42

Closed hirooih closed 4 years ago

hirooih commented 4 years ago

These are my proposals.

Most of them are for clarifications which I thought I needed when I read this guide. If you have any questions, feel free to ask me.

I'm new to git and GitHub. I read documentations on Webs. For example I tried to commit on each topic. But I might be doing wrong. I welcome your suggestions.

Thanks.

hirooih commented 4 years ago

@imphil , thank you for your comments.

In the interest of getting this PR in, would you mind splitting things up?

Sure.

Remove the added paragraph about clock dividers from this PR,

I did on https://github.com/lowRISC/style-guides/pull/42/commits/0c28912050e7e2d908ed1fe216b5bb2b82dad458

and file a follow-up issue where we can discuss this guidance.

I found that I made a mistake. I did not understand a branch for Git (it is different from a branch for other SCMs) and did not create a branch for this pull-request. I tried to make a branch and a new pull request for the clock divider issue. But it includes whole changes for this pull-request. I will open a new pull-request for clock divider after this request is merged.

sjgitty commented 4 years ago

I think this is ready to go in as written. Thanks @hirooih for the clarifications. @imphil I'll give you one last chance to weigh in else I'll commit later today. Thanks.

imphil commented 4 years ago

Thanks a lot for those changes @hirooih. I took the liberty to combine your commits into one and commit them directly into the master branch (otherwise you'd have to rebase this pull request, which is a bit tricky if you're new to git).

Regarding the race condition topic: please feel free to open a new issue here in the repository and we can discuss it there. (Or in other words: it's not a rejected proposal, it's only one which needs more discussion to really understand what is going on.)