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

refactor: s/regs/registers #1105

Merged
merged 2 commits into from Jul 13, 2018

Conversation

Projects
None yet
7 participants
@ppannuto
Copy link
Member

ppannuto commented Jul 9, 2018

Pull Request Overview

Following a brief IRC discussion, update regs to registers
https://bot.tockos.org/tockbot/tock/2018-07-09/?msg=111467&page=1

Summary of IRC:

I’m pretty adverse to needless abbreviations, and despite the folder name tock-register-interface everything internally is tock-regs and examples use regs. I’m inclined to change this to tock-registers and registers before publishing.
+1's from Brad and Amit

Testing Strategy

Compiling.

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make formatall.

@ppannuto ppannuto force-pushed the regs-to-registers branch from 976b114 to bdcbef2 Jul 9, 2018

@brghena

This comment has been minimized.

Copy link
Contributor

brghena commented Jul 9, 2018

@andre-richter I think this will require changes on your end if merged.

@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jul 9, 2018

@andre-richter Yes, it will -- it's a step towards publishing on crates.io, I'll send an email out-of-band once it's published summarizing any changes (probably just this).

@bradjc
Copy link
Contributor

bradjc left a comment

Ok that's a lot of changes. Can you send one that is just the crates.io changes?

@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jul 9, 2018

I intend to do a PR for crates.io stuff next, this is just a s/regs/registers, it doesn't discriminate one way or another for the tock-regs stuff

There are no crates.io changes in this PR

@ppannuto ppannuto referenced this pull request Jul 10, 2018

Merged

Updates to tock-registers for publishing #1106

2 of 2 tasks complete
@bradjc
Copy link
Contributor

bradjc left a comment

I would rather just leave the chip files how they are rather than make changes across 66 files.

@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jul 10, 2018

I think @bradjc and I have reached an impasse.

I would rather see a consistent use of regs vs registers throughout the codebase, rather than naming some things regs and others registers.

I would prefer to use words rather than arbitrary abbreviations. I think that's been agreed to for the tock-registers library, but not for local variable names.

Anyone else care to weigh in?

@niklasad1
Copy link
Member

niklasad1 left a comment

LGTM!

@andre-richter

This comment has been minimized.

Copy link
Contributor

andre-richter commented Jul 10, 2018

👍

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 10, 2018

@ppannuto can you briefly summarize the IRC discussion rather than just link to it?

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 10, 2018

I don't care strongly one way or another, but I'm sympathetic to @bradjc's concern about changing so many files on a whim (and the kernel interface for that matter).

Since we're re-exporting the registers interface from the kernel anyway, is a reasonable middle-ground to change the names in the tock-registers crate, but re-export the symbols as ...::regs::... from the kernel crate to the rest of Tock?

@brghena

This comment has been minimized.

Copy link
Contributor

brghena commented Jul 10, 2018

I don't feel strongly about this change either. Generally it's better to spell things out rather than abbreviate, but this mostly just feels like code churn to me.

On the consistency side, can you point to a problematic case? A grep through the codebase shows me that a lot of files have a self.registers member that they refer to as let regs = &*self.registers within code blocks. I don't mind this shorthand and don't think it's all that confusing. The one exception to this may be the SAM4L DMA, which calls let registers = &*self.registers, but it is consistent internally.

One source of concern, it seemed like @alevy and @bradjc were fine with this change when proposed on IRC. Was there something in particular that changed in this implementation from what you were imagining (maybe just the scale)? We should definitely avoid spending effort on things that aren't going to be wanted.

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 10, 2018

Nothing changed for me. I'm just over here not really caring and trying to figure out how to turn a "requested changes" into an "approved these changes"

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 10, 2018

I thought the change proposed on irc was just renaming the crate to tock-registers, and updating the extern line in the kernel crate.

@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jul 10, 2018

Why does it matter that 66 files change? What matters is what kind of change. If we decided to add a license to every source file we'd change every one of them. If it relates to package names, so be it.

That being said, going through lots of files written by other people and changing their variable names because one doesn't like them isn't great behavior.

refactor: s/regs/registers
Following a brief IRC discussion, update regs to registers
https://bot.tockos.org/tockbot/tock/2018-07-09/?msg=111467&page=1

Summary of IRC:

> I'm pretty adverse to needless abbreviations, and despite the folder name
> tock-register-interface everything internally is tock-regs and examples use
> regs. I'm inclined to change this to tock-registers and registers before
> publishing.
>
> +1's from Brad and Amit

Then revised in discussion in #1105 to only rename some instances
of regs to registers, which I disagree with, but don't care enough
to fight about.

@ppannuto ppannuto dismissed stale reviews from niklasad1 and brghena via 646de24 Jul 12, 2018

@ppannuto ppannuto force-pushed the regs-to-registers branch from bdcbef2 to 646de24 Jul 12, 2018

Pruned all local variable renames

@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jul 12, 2018

I'll echo @brghena's final point: my primary reticence came from essentially making the same changes twice, but I've done that now so hopefully this can move through this time.

@phil-levis
Copy link
Collaborator

phil-levis left a comment

Seems fine. Changing all of the variable names to registers seems not great, but I wouldn't block on it.

@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jul 13, 2018

Just a merge conflict resolution.

@ppannuto ppannuto merged commit 8dc28f3 into master Jul 13, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@ppannuto ppannuto deleted the regs-to-registers branch Jul 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment