Cleanup: rename middle::ty::sty and its variants. - #26232

Merged
bors merged 1 commit into
rust-lang:masterfrom
eefriedman:rename-sty
Jun 12, 2015
Merged

Cleanup: rename middle::ty::sty and its variants.#26232
bors merged 1 commit into
rust-lang:masterfrom
eefriedman:rename-sty

Conversation

@eefriedman

@eefriedmaneefriedman commented Jun 12, 2015

Copy link
Copy Markdown
Contributor

Use camel-case naming, and use names which actually make sense in modern Rust.

@rust-highfive

rust-highfive commented Jun 12, 2015

Copy link
Copy Markdown
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nrc (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@bors

bors commented Jun 12, 2015

Copy link
Copy Markdown
Collaborator

☔ The latest upstream changes (presumably #26225) made this pull request unmergeable. Please resolve the merge conflicts.

@eddyb

eddyb commented Jun 12, 2015

Copy link
Copy Markdown
Contributor

LGTM, but a could of names seem somewhat long: TyReference and TyArrayOrSlice.
Another thought is that the Ty prefix could be dropped ifTy::Foo could be made to work instead (right now it's not looking so great).

@eefriedman

eefriedman commented Jun 12, 2015

Copy link
Copy Markdown
ContributorAuthor

I can shorten TyReference to TyRef without much loss of clarity.

I don't really know how to shorten TyArrayOrSlice; TyVec like master uses is misleading, neither TySquare or TyAOrS is obvious, and I can't think of anything else. I'm thinking it will get split into TyArray and TySlice in a followup, so maybe just leave it for now?

As for getting rid of the Ty prefix, it might be possible to rename TypeVariants to something shorter like TyV, sprinkle some use rustc::middle::ty::TyV into other modules, and write TyV::Int instead of ty::TyInt. Not sure if that's worthwhile.

@nrc

nrc commented Jun 12, 2015

Copy link
Copy Markdown
Member

In general, I don't mind long names so much. TyRef is fine. I think TyArray would be fine instead of TyArrayOrSlice - it is close enough. Dropping the Ty prefix in favour of Ty:: would be nice. But I don't want to see TyV at all - that's indecipherable. (In general, we have a history of short and somewhat bad names in the compiler and we've been moving towards longer, more understandable names. Obviously there are specific places where concision is still warranted though).

nrc
nrc reviewed Jun 12, 2015
View reviewed changes
Comment threadsrc/librustc/middle/ty.rs

@nrcnrcJun 12, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

maybe Types rather than TyV or TypeVariants?

@eddybeddybJun 12, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think @nikomatsakis also mentioned TyKind a long time ago (when I considered making this cleanup).

@nrc

nrc commented Jun 12, 2015

Copy link
Copy Markdown
Member

This is a great PR, I've wanted to rename these things for a while. Thanks for making the changes. If you could at least change TyArrayOrSlice and TyReference we could land this. You could try changing to use the prefix, or we could land this and do that as a follow up - I realise this PR will rot quite quickly.

@eefriedman
eefriedmanforce-pushed the rename-sty branch 2 times, most recently from 877fef6 to 9fad9afCompareJune 12, 2015 04:40
@eefriedman

eefriedman commented Jun 12, 2015

Copy link
Copy Markdown
ContributorAuthor

Rebased, and amended to use TyRef and TyArray.

I think I'll experiment with the prefixes later; there's already a variant named "Types" in librustc/middle/infer/mod.rs.

@nrc

nrc commented Jun 12, 2015

Copy link
Copy Markdown
Member

@bors: r+ p=1

@bors

bors commented Jun 12, 2015

Copy link
Copy Markdown
Collaborator

📌 Commit 9fad9af has been approved by nrc

@bors

bors commented Jun 12, 2015

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 9fad9af with merge 7626e9f...

@bors

bors commented Jun 12, 2015

Copy link
Copy Markdown
Collaborator

💔 Test failed - auto-linux-64-nopt-t

@eefriedman

eefriedman commented Jun 12, 2015

Copy link
Copy Markdown
ContributorAuthor

I didn't rebase correctly; sorry about the churn. Amended with fix.

@nrc

nrc commented Jun 12, 2015

Copy link
Copy Markdown
Member

@bors: r+ p=1

@bors

bors commented Jun 12, 2015

Copy link
Copy Markdown
Collaborator

📌 Commit 00f86fe has been approved by nrc

@bors

bors commented Jun 12, 2015

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 00f86fe with merge f4bdebf...

bors added a commit that referenced this pull request Jun 12, 2015
@bors
Auto merge of#26232- eefriedman:rename-sty, r=nrc
f4bdebf
Use camel-case naming, and use names which actually make sense in modern Rust.
@bors

bors commented Jun 12, 2015

Copy link
Copy Markdown
Collaborator

💔 Test failed - auto-mac-64-opt

@eefriedman
Cleanup: rename middle::ty::sty and its variants.
3c69db4
Use camel-case naming, and use names which actually make sense in modern Rust.
@eefriedman

eefriedman commented Jun 12, 2015

Copy link
Copy Markdown
ContributorAuthor

Updated again; I thought I had built everything, but I somehow missed librustdoc.

@eddyb

eddyb commented Jun 12, 2015

Copy link
Copy Markdown
Contributor

@bors r=nrc

@bors

bors commented Jun 12, 2015

Copy link
Copy Markdown
Collaborator

📌 Commit 3c69db4 has been approved by nrc

@bors

bors commented Jun 12, 2015

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 3c69db4 with merge 85b5338...

bors added a commit that referenced this pull request Jun 12, 2015
@bors
Auto merge of#26232- eefriedman:rename-sty, r=nrc
85b5338
Use camel-case naming, and use names which actually make sense in modern Rust.
@bors

bors commented Jun 12, 2015

Copy link
Copy Markdown
Collaborator

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt

@bors
bors merged commit 3c69db4 into rust-lang:masterJun 12, 2015
This was referenced Jun 12, 2015
Closed
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@nrcnrc

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@eefriedman@rust-highfive@bors@eddyb@nrc
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Cleanup: rename middle::ty::sty and its variants. - #26232
Merged
bors merged 1 commit into
rust-lang:masterfrom 
eefriedman:rename-sty
Jun 12, 2015
Merged
Cleanup: rename middle::ty::sty and its variants.#26232
bors merged 1 commit into
rust-lang:masterfrom 
eefriedman:rename-sty
Conversation
@eefriedman
@eefriedmaneefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

Contributor
Use camel-case naming, and use names which actually make sense in modern Rust.
@rust-highfive
rust-highfive
commented
Jun 12, 2015

Copy link


Copy Markdown

Contributor
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nrc (or someone else) soon.
If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.
Please see the contribution instructions for more information.
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
☔ The latest upstream changes (presumably #26225) made this pull request unmergeable. Please resolve the merge conflicts.
@eddyb
eddyb
commented
Jun 12, 2015

Copy link


Copy Markdown

Contributor
LGTM, but a could of names seem somewhat long: TyReference and TyArrayOrSlice.

Another thought is that the Ty prefix could be dropped ifTy::Foo could be made to work instead (right now it's not looking so great).
@eefriedman
eefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

ContributorAuthor
I can shorten TyReference to TyRef without much loss of clarity.
I don't really know how to shorten TyArrayOrSlice; TyVec like master uses is misleading, neither TySquare or TyAOrS is obvious, and I can't think of anything else. I'm thinking it will get split into TyArray and TySlice in a followup, so maybe just leave it for now?
As for getting rid of the Ty prefix, it might be possible to rename TypeVariants to something shorter like TyV, sprinkle some use rustc::middle::ty::TyV into other modules, and write TyV::Int instead of ty::TyInt. Not sure if that's worthwhile.
@nrc
nrc
commented
Jun 12, 2015

Copy link


Copy Markdown

Member
In general, I don't mind long names so much. TyRef is fine. I think TyArray would be fine instead of TyArrayOrSlice - it is close enough. Dropping the Ty prefix in favour of Ty:: would be nice. But I don't want to see TyV at all - that's indecipherable. (In general, we have a history of short and somewhat bad names in the compiler and we've been moving towards longer, more understandable names. Obviously there are specific places where concision is still warranted though).
nrc
nrc
reviewed
Jun 12, 2015
 View reviewed changes

Comment threadsrc/librustc/middle/ty.rs
@nrcnrcJun 12, 2015

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

maybe Types rather than TyV or TypeVariants?
@eddybeddybJun 12, 2015

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think @nikomatsakis also mentioned TyKind a long time ago (when I considered making this cleanup).
@nrc
nrc
commented
Jun 12, 2015

Copy link


Copy Markdown

Member
This is a great PR, I've wanted to rename these things for a while. Thanks for making the changes. If you could at least change TyArrayOrSlice and TyReference we could land this. You could try changing to use the prefix, or we could land this and do that as a follow up - I realise this PR will rot quite quickly.
@eefriedman
eefriedmanforce-pushed
the
rename-sty
branch
2 times, most recently
from
877fef6 to
9fad9afCompareJune 12, 2015 04:40
@eefriedman
eefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

ContributorAuthor
Rebased, and amended to use TyRef and TyArray.
I think I'll experiment with the prefixes later; there's already a variant named "Types" in librustc/middle/infer/mod.rs.
@nrc
nrc
commented
Jun 12, 2015

Copy link


Copy Markdown

Member
@bors: r+ p=1
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
📌 Commit 9fad9af has been approved by nrc
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
⌛ Testing commit 9fad9af with merge 7626e9f...
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
💔 Test failed - auto-linux-64-nopt-t
@eefriedman
eefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

ContributorAuthor
I didn't rebase correctly; sorry about the churn. Amended with fix.
@nrc
nrc
commented
Jun 12, 2015

Copy link


Copy Markdown

Member
@bors: r+ p=1
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
📌 Commit 00f86fe has been approved by nrc
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
⌛ Testing commit 00f86fe with merge f4bdebf...
bors added a commit
that referenced
this pull request
Jun 12, 2015
@bors
Auto merge of#26232- eefriedman:rename-sty, r=nrc
f4bdebf
Use camel-case naming, and use names which actually make sense in modern Rust.
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
💔 Test failed - auto-mac-64-opt
@eefriedman
Cleanup: rename middle::ty::sty and its variants.
3c69db4
Use camel-case naming, and use names which actually make sense in modern Rust.
@eefriedman
eefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

ContributorAuthor
Updated again; I thought I had built everything, but I somehow missed librustdoc.
@eddyb
eddyb
commented
Jun 12, 2015

Copy link


Copy Markdown

Contributor
@bors r=nrc
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
📌 Commit 3c69db4 has been approved by nrc
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
⌛ Testing commit 3c69db4 with merge 85b5338...
bors added a commit
that referenced
this pull request
Jun 12, 2015
@bors
Auto merge of#26232- eefriedman:rename-sty, r=nrc
85b5338
Use camel-case naming, and use names which actually make sense in modern Rust.
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt
@bors
bors
merged commit 3c69db4
into
rust-lang:masterJun 12, 2015

This was referenced Jun 12, 2015
 Closed

 Merged

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

Reviewers


No reviews


Assignees

@nrcnrc

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@eefriedman@rust-highfive@bors@eddyb@nrc

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Cleanup: rename middle::ty::sty and its variants. - #26232
Merged
bors merged 1 commit into
rust-lang:masterfrom 
eefriedman:rename-sty
Jun 12, 2015
Merged
Cleanup: rename middle::ty::sty and its variants.#26232
bors merged 1 commit into
rust-lang:masterfrom 
eefriedman:rename-sty
Conversation
@eefriedman
@eefriedmaneefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

Contributor
Use camel-case naming, and use names which actually make sense in modern Rust.
@rust-highfive
rust-highfive
commented
Jun 12, 2015

Copy link


Copy Markdown

Contributor
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nrc (or someone else) soon.
If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.
Please see the contribution instructions for more information.
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
☔ The latest upstream changes (presumably #26225) made this pull request unmergeable. Please resolve the merge conflicts.
@eddyb
eddyb
commented
Jun 12, 2015

Copy link


Copy Markdown

Contributor
LGTM, but a could of names seem somewhat long: TyReference and TyArrayOrSlice.

Another thought is that the Ty prefix could be dropped ifTy::Foo could be made to work instead (right now it's not looking so great).
@eefriedman
eefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

ContributorAuthor
I can shorten TyReference to TyRef without much loss of clarity.
I don't really know how to shorten TyArrayOrSlice; TyVec like master uses is misleading, neither TySquare or TyAOrS is obvious, and I can't think of anything else. I'm thinking it will get split into TyArray and TySlice in a followup, so maybe just leave it for now?
As for getting rid of the Ty prefix, it might be possible to rename TypeVariants to something shorter like TyV, sprinkle some use rustc::middle::ty::TyV into other modules, and write TyV::Int instead of ty::TyInt. Not sure if that's worthwhile.
@nrc
nrc
commented
Jun 12, 2015

Copy link


Copy Markdown

Member
In general, I don't mind long names so much. TyRef is fine. I think TyArray would be fine instead of TyArrayOrSlice - it is close enough. Dropping the Ty prefix in favour of Ty:: would be nice. But I don't want to see TyV at all - that's indecipherable. (In general, we have a history of short and somewhat bad names in the compiler and we've been moving towards longer, more understandable names. Obviously there are specific places where concision is still warranted though).
nrc
nrc
reviewed
Jun 12, 2015
 View reviewed changes

Comment threadsrc/librustc/middle/ty.rs
@nrcnrcJun 12, 2015

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

maybe Types rather than TyV or TypeVariants?
@eddybeddybJun 12, 2015

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think @nikomatsakis also mentioned TyKind a long time ago (when I considered making this cleanup).
@nrc
nrc
commented
Jun 12, 2015

Copy link


Copy Markdown

Member
This is a great PR, I've wanted to rename these things for a while. Thanks for making the changes. If you could at least change TyArrayOrSlice and TyReference we could land this. You could try changing to use the prefix, or we could land this and do that as a follow up - I realise this PR will rot quite quickly.
@eefriedman
eefriedmanforce-pushed
the
rename-sty
branch
2 times, most recently
from
877fef6 to
9fad9afCompareJune 12, 2015 04:40
@eefriedman
eefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

ContributorAuthor
Rebased, and amended to use TyRef and TyArray.
I think I'll experiment with the prefixes later; there's already a variant named "Types" in librustc/middle/infer/mod.rs.
@nrc
nrc
commented
Jun 12, 2015

Copy link


Copy Markdown

Member
@bors: r+ p=1
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
📌 Commit 9fad9af has been approved by nrc
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
⌛ Testing commit 9fad9af with merge 7626e9f...
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
💔 Test failed - auto-linux-64-nopt-t
@eefriedman
eefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

ContributorAuthor
I didn't rebase correctly; sorry about the churn. Amended with fix.
@nrc
nrc
commented
Jun 12, 2015

Copy link


Copy Markdown

Member
@bors: r+ p=1
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
📌 Commit 00f86fe has been approved by nrc
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
⌛ Testing commit 00f86fe with merge f4bdebf...
bors added a commit
that referenced
this pull request
Jun 12, 2015
@bors
Auto merge of#26232- eefriedman:rename-sty, r=nrc
f4bdebf
Use camel-case naming, and use names which actually make sense in modern Rust.
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
💔 Test failed - auto-mac-64-opt
@eefriedman
Cleanup: rename middle::ty::sty and its variants.
3c69db4
Use camel-case naming, and use names which actually make sense in modern Rust.
@eefriedman
eefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

ContributorAuthor
Updated again; I thought I had built everything, but I somehow missed librustdoc.
@eddyb
eddyb
commented
Jun 12, 2015

Copy link


Copy Markdown

Contributor
@bors r=nrc
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
📌 Commit 3c69db4 has been approved by nrc
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
⌛ Testing commit 3c69db4 with merge 85b5338...
bors added a commit
that referenced
this pull request
Jun 12, 2015
@bors
Auto merge of#26232- eefriedman:rename-sty, r=nrc
85b5338
Use camel-case naming, and use names which actually make sense in modern Rust.
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt
@bors
bors
merged commit 3c69db4
into
rust-lang:masterJun 12, 2015

This was referenced Jun 12, 2015
 Closed

 Merged

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

Reviewers


No reviews


Assignees

@nrcnrc

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@eefriedman@rust-highfive@bors@eddyb@nrc

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Cleanup: rename middle::ty::sty and its variants. - #26232
Merged
bors merged 1 commit into
rust-lang:masterfrom 
eefriedman:rename-sty
Jun 12, 2015
Merged
Cleanup: rename middle::ty::sty and its variants.#26232
bors merged 1 commit into
rust-lang:masterfrom 
eefriedman:rename-sty
Conversation
@eefriedman
@eefriedmaneefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

Contributor
Use camel-case naming, and use names which actually make sense in modern Rust.
@rust-highfive
rust-highfive
commented
Jun 12, 2015

Copy link


Copy Markdown

Contributor
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nrc (or someone else) soon.
If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.
Please see the contribution instructions for more information.
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
☔ The latest upstream changes (presumably #26225) made this pull request unmergeable. Please resolve the merge conflicts.
@eddyb
eddyb
commented
Jun 12, 2015

Copy link


Copy Markdown

Contributor
LGTM, but a could of names seem somewhat long: TyReference and TyArrayOrSlice.

Another thought is that the Ty prefix could be dropped ifTy::Foo could be made to work instead (right now it's not looking so great).
@eefriedman
eefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

ContributorAuthor
I can shorten TyReference to TyRef without much loss of clarity.
I don't really know how to shorten TyArrayOrSlice; TyVec like master uses is misleading, neither TySquare or TyAOrS is obvious, and I can't think of anything else. I'm thinking it will get split into TyArray and TySlice in a followup, so maybe just leave it for now?
As for getting rid of the Ty prefix, it might be possible to rename TypeVariants to something shorter like TyV, sprinkle some use rustc::middle::ty::TyV into other modules, and write TyV::Int instead of ty::TyInt. Not sure if that's worthwhile.
@nrc
nrc
commented
Jun 12, 2015

Copy link


Copy Markdown

Member
In general, I don't mind long names so much. TyRef is fine. I think TyArray would be fine instead of TyArrayOrSlice - it is close enough. Dropping the Ty prefix in favour of Ty:: would be nice. But I don't want to see TyV at all - that's indecipherable. (In general, we have a history of short and somewhat bad names in the compiler and we've been moving towards longer, more understandable names. Obviously there are specific places where concision is still warranted though).
nrc
nrc
reviewed
Jun 12, 2015
 View reviewed changes

Comment threadsrc/librustc/middle/ty.rs
@nrcnrcJun 12, 2015

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

maybe Types rather than TyV or TypeVariants?
@eddybeddybJun 12, 2015

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think @nikomatsakis also mentioned TyKind a long time ago (when I considered making this cleanup).
@nrc
nrc
commented
Jun 12, 2015

Copy link


Copy Markdown

Member
This is a great PR, I've wanted to rename these things for a while. Thanks for making the changes. If you could at least change TyArrayOrSlice and TyReference we could land this. You could try changing to use the prefix, or we could land this and do that as a follow up - I realise this PR will rot quite quickly.
@eefriedman
eefriedmanforce-pushed
the
rename-sty
branch
2 times, most recently
from
877fef6 to
9fad9afCompareJune 12, 2015 04:40
@eefriedman
eefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

ContributorAuthor
Rebased, and amended to use TyRef and TyArray.
I think I'll experiment with the prefixes later; there's already a variant named "Types" in librustc/middle/infer/mod.rs.
@nrc
nrc
commented
Jun 12, 2015

Copy link


Copy Markdown

Member
@bors: r+ p=1
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
📌 Commit 9fad9af has been approved by nrc
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
⌛ Testing commit 9fad9af with merge 7626e9f...
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
💔 Test failed - auto-linux-64-nopt-t
@eefriedman
eefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

ContributorAuthor
I didn't rebase correctly; sorry about the churn. Amended with fix.
@nrc
nrc
commented
Jun 12, 2015

Copy link


Copy Markdown

Member
@bors: r+ p=1
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
📌 Commit 00f86fe has been approved by nrc
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
⌛ Testing commit 00f86fe with merge f4bdebf...
bors added a commit
that referenced
this pull request
Jun 12, 2015
@bors
Auto merge of#26232- eefriedman:rename-sty, r=nrc
f4bdebf
Use camel-case naming, and use names which actually make sense in modern Rust.
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
💔 Test failed - auto-mac-64-opt
@eefriedman
Cleanup: rename middle::ty::sty and its variants.
3c69db4
Use camel-case naming, and use names which actually make sense in modern Rust.
@eefriedman
eefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

ContributorAuthor
Updated again; I thought I had built everything, but I somehow missed librustdoc.
@eddyb
eddyb
commented
Jun 12, 2015

Copy link


Copy Markdown

Contributor
@bors r=nrc
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
📌 Commit 3c69db4 has been approved by nrc
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
⌛ Testing commit 3c69db4 with merge 85b5338...
bors added a commit
that referenced
this pull request
Jun 12, 2015
@bors
Auto merge of#26232- eefriedman:rename-sty, r=nrc
85b5338
Use camel-case naming, and use names which actually make sense in modern Rust.
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt
@bors
bors
merged commit 3c69db4
into
rust-lang:masterJun 12, 2015

This was referenced Jun 12, 2015
 Closed

 Merged

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

Reviewers


No reviews


Assignees

@nrcnrc

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@eefriedman@rust-highfive@bors@eddyb@nrc

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Cleanup: rename middle::ty::sty and its variants. - #26232
Merged
bors merged 1 commit into
rust-lang:masterfrom 
eefriedman:rename-sty
Jun 12, 2015
Merged
Cleanup: rename middle::ty::sty and its variants.#26232
bors merged 1 commit into
rust-lang:masterfrom 
eefriedman:rename-sty
Conversation
@eefriedman
@eefriedmaneefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

Contributor
Use camel-case naming, and use names which actually make sense in modern Rust.
@rust-highfive
rust-highfive
commented
Jun 12, 2015

Copy link


Copy Markdown

Contributor
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nrc (or someone else) soon.
If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.
Please see the contribution instructions for more information.
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
☔ The latest upstream changes (presumably #26225) made this pull request unmergeable. Please resolve the merge conflicts.
@eddyb
eddyb
commented
Jun 12, 2015

Copy link


Copy Markdown

Contributor
LGTM, but a could of names seem somewhat long: TyReference and TyArrayOrSlice.

Another thought is that the Ty prefix could be dropped ifTy::Foo could be made to work instead (right now it's not looking so great).
@eefriedman
eefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

ContributorAuthor
I can shorten TyReference to TyRef without much loss of clarity.
I don't really know how to shorten TyArrayOrSlice; TyVec like master uses is misleading, neither TySquare or TyAOrS is obvious, and I can't think of anything else. I'm thinking it will get split into TyArray and TySlice in a followup, so maybe just leave it for now?
As for getting rid of the Ty prefix, it might be possible to rename TypeVariants to something shorter like TyV, sprinkle some use rustc::middle::ty::TyV into other modules, and write TyV::Int instead of ty::TyInt. Not sure if that's worthwhile.
@nrc
nrc
commented
Jun 12, 2015

Copy link


Copy Markdown

Member
In general, I don't mind long names so much. TyRef is fine. I think TyArray would be fine instead of TyArrayOrSlice - it is close enough. Dropping the Ty prefix in favour of Ty:: would be nice. But I don't want to see TyV at all - that's indecipherable. (In general, we have a history of short and somewhat bad names in the compiler and we've been moving towards longer, more understandable names. Obviously there are specific places where concision is still warranted though).
nrc
nrc
reviewed
Jun 12, 2015
 View reviewed changes

Comment threadsrc/librustc/middle/ty.rs
@nrcnrcJun 12, 2015

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

maybe Types rather than TyV or TypeVariants?
@eddybeddybJun 12, 2015

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think @nikomatsakis also mentioned TyKind a long time ago (when I considered making this cleanup).
@nrc
nrc
commented
Jun 12, 2015

Copy link


Copy Markdown

Member
This is a great PR, I've wanted to rename these things for a while. Thanks for making the changes. If you could at least change TyArrayOrSlice and TyReference we could land this. You could try changing to use the prefix, or we could land this and do that as a follow up - I realise this PR will rot quite quickly.
@eefriedman
eefriedmanforce-pushed
the
rename-sty
branch
2 times, most recently
from
877fef6 to
9fad9afCompareJune 12, 2015 04:40
@eefriedman
eefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

ContributorAuthor
Rebased, and amended to use TyRef and TyArray.
I think I'll experiment with the prefixes later; there's already a variant named "Types" in librustc/middle/infer/mod.rs.
@nrc
nrc
commented
Jun 12, 2015

Copy link


Copy Markdown

Member
@bors: r+ p=1
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
📌 Commit 9fad9af has been approved by nrc
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
⌛ Testing commit 9fad9af with merge 7626e9f...
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
💔 Test failed - auto-linux-64-nopt-t
@eefriedman
eefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

ContributorAuthor
I didn't rebase correctly; sorry about the churn. Amended with fix.
@nrc
nrc
commented
Jun 12, 2015

Copy link


Copy Markdown

Member
@bors: r+ p=1
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
📌 Commit 00f86fe has been approved by nrc
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
⌛ Testing commit 00f86fe with merge f4bdebf...
bors added a commit
that referenced
this pull request
Jun 12, 2015
@bors
Auto merge of#26232- eefriedman:rename-sty, r=nrc
f4bdebf
Use camel-case naming, and use names which actually make sense in modern Rust.
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
💔 Test failed - auto-mac-64-opt
@eefriedman
Cleanup: rename middle::ty::sty and its variants.
3c69db4
Use camel-case naming, and use names which actually make sense in modern Rust.
@eefriedman
eefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

ContributorAuthor
Updated again; I thought I had built everything, but I somehow missed librustdoc.
@eddyb
eddyb
commented
Jun 12, 2015

Copy link


Copy Markdown

Contributor
@bors r=nrc
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
📌 Commit 3c69db4 has been approved by nrc
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
⌛ Testing commit 3c69db4 with merge 85b5338...
bors added a commit
that referenced
this pull request
Jun 12, 2015
@bors
Auto merge of#26232- eefriedman:rename-sty, r=nrc
85b5338
Use camel-case naming, and use names which actually make sense in modern Rust.
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt
@bors
bors
merged commit 3c69db4
into
rust-lang:masterJun 12, 2015

This was referenced Jun 12, 2015
 Closed

 Merged

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

Reviewers


No reviews


Assignees

@nrcnrc

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@eefriedman@rust-highfive@bors@eddyb@nrc

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Cleanup: rename middle::ty::sty and its variants. - #26232
Merged
bors merged 1 commit into
rust-lang:masterfrom 
eefriedman:rename-sty
Jun 12, 2015
Merged
Cleanup: rename middle::ty::sty and its variants.#26232
bors merged 1 commit into
rust-lang:masterfrom 
eefriedman:rename-sty
Conversation
@eefriedman
@eefriedmaneefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

Contributor
Use camel-case naming, and use names which actually make sense in modern Rust.
@rust-highfive
rust-highfive
commented
Jun 12, 2015

Copy link


Copy Markdown

Contributor
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nrc (or someone else) soon.
If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.
Please see the contribution instructions for more information.
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
☔ The latest upstream changes (presumably #26225) made this pull request unmergeable. Please resolve the merge conflicts.
@eddyb
eddyb
commented
Jun 12, 2015

Copy link


Copy Markdown

Contributor
LGTM, but a could of names seem somewhat long: TyReference and TyArrayOrSlice.

Another thought is that the Ty prefix could be dropped ifTy::Foo could be made to work instead (right now it's not looking so great).
@eefriedman
eefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

ContributorAuthor
I can shorten TyReference to TyRef without much loss of clarity.
I don't really know how to shorten TyArrayOrSlice; TyVec like master uses is misleading, neither TySquare or TyAOrS is obvious, and I can't think of anything else. I'm thinking it will get split into TyArray and TySlice in a followup, so maybe just leave it for now?
As for getting rid of the Ty prefix, it might be possible to rename TypeVariants to something shorter like TyV, sprinkle some use rustc::middle::ty::TyV into other modules, and write TyV::Int instead of ty::TyInt. Not sure if that's worthwhile.
@nrc
nrc
commented
Jun 12, 2015

Copy link


Copy Markdown

Member
In general, I don't mind long names so much. TyRef is fine. I think TyArray would be fine instead of TyArrayOrSlice - it is close enough. Dropping the Ty prefix in favour of Ty:: would be nice. But I don't want to see TyV at all - that's indecipherable. (In general, we have a history of short and somewhat bad names in the compiler and we've been moving towards longer, more understandable names. Obviously there are specific places where concision is still warranted though).
nrc
nrc
reviewed
Jun 12, 2015
 View reviewed changes

Comment threadsrc/librustc/middle/ty.rs
@nrcnrcJun 12, 2015

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

maybe Types rather than TyV or TypeVariants?
@eddybeddybJun 12, 2015

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think @nikomatsakis also mentioned TyKind a long time ago (when I considered making this cleanup).
@nrc
nrc
commented
Jun 12, 2015

Copy link


Copy Markdown

Member
This is a great PR, I've wanted to rename these things for a while. Thanks for making the changes. If you could at least change TyArrayOrSlice and TyReference we could land this. You could try changing to use the prefix, or we could land this and do that as a follow up - I realise this PR will rot quite quickly.
@eefriedman
eefriedmanforce-pushed
the
rename-sty
branch
2 times, most recently
from
877fef6 to
9fad9afCompareJune 12, 2015 04:40
@eefriedman
eefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

ContributorAuthor
Rebased, and amended to use TyRef and TyArray.
I think I'll experiment with the prefixes later; there's already a variant named "Types" in librustc/middle/infer/mod.rs.
@nrc
nrc
commented
Jun 12, 2015

Copy link


Copy Markdown

Member
@bors: r+ p=1
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
📌 Commit 9fad9af has been approved by nrc
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
⌛ Testing commit 9fad9af with merge 7626e9f...
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
💔 Test failed - auto-linux-64-nopt-t
@eefriedman
eefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

ContributorAuthor
I didn't rebase correctly; sorry about the churn. Amended with fix.
@nrc
nrc
commented
Jun 12, 2015

Copy link


Copy Markdown

Member
@bors: r+ p=1
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
📌 Commit 00f86fe has been approved by nrc
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
⌛ Testing commit 00f86fe with merge f4bdebf...
bors added a commit
that referenced
this pull request
Jun 12, 2015
@bors
Auto merge of#26232- eefriedman:rename-sty, r=nrc
f4bdebf
Use camel-case naming, and use names which actually make sense in modern Rust.
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
💔 Test failed - auto-mac-64-opt
@eefriedman
Cleanup: rename middle::ty::sty and its variants.
3c69db4
Use camel-case naming, and use names which actually make sense in modern Rust.
@eefriedman
eefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

ContributorAuthor
Updated again; I thought I had built everything, but I somehow missed librustdoc.
@eddyb
eddyb
commented
Jun 12, 2015

Copy link


Copy Markdown

Contributor
@bors r=nrc
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
📌 Commit 3c69db4 has been approved by nrc
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
⌛ Testing commit 3c69db4 with merge 85b5338...
bors added a commit
that referenced
this pull request
Jun 12, 2015
@bors
Auto merge of#26232- eefriedman:rename-sty, r=nrc
85b5338
Use camel-case naming, and use names which actually make sense in modern Rust.
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt
@bors
bors
merged commit 3c69db4
into
rust-lang:masterJun 12, 2015

This was referenced Jun 12, 2015
 Closed

 Merged

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

Reviewers


No reviews


Assignees

@nrcnrc

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@eefriedman@rust-highfive@bors@eddyb@nrc

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Cleanup: rename middle::ty::sty and its variants. - #26232
Merged
bors merged 1 commit into
rust-lang:masterfrom 
eefriedman:rename-sty
Jun 12, 2015
Merged
Cleanup: rename middle::ty::sty and its variants.#26232
bors merged 1 commit into
rust-lang:masterfrom 
eefriedman:rename-sty
Conversation
@eefriedman
@eefriedmaneefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

Contributor
Use camel-case naming, and use names which actually make sense in modern Rust.
@rust-highfive
rust-highfive
commented
Jun 12, 2015

Copy link


Copy Markdown

Contributor
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nrc (or someone else) soon.
If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.
Please see the contribution instructions for more information.
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
☔ The latest upstream changes (presumably #26225) made this pull request unmergeable. Please resolve the merge conflicts.
@eddyb
eddyb
commented
Jun 12, 2015

Copy link


Copy Markdown

Contributor
LGTM, but a could of names seem somewhat long: TyReference and TyArrayOrSlice.

Another thought is that the Ty prefix could be dropped ifTy::Foo could be made to work instead (right now it's not looking so great).
@eefriedman
eefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

ContributorAuthor
I can shorten TyReference to TyRef without much loss of clarity.
I don't really know how to shorten TyArrayOrSlice; TyVec like master uses is misleading, neither TySquare or TyAOrS is obvious, and I can't think of anything else. I'm thinking it will get split into TyArray and TySlice in a followup, so maybe just leave it for now?
As for getting rid of the Ty prefix, it might be possible to rename TypeVariants to something shorter like TyV, sprinkle some use rustc::middle::ty::TyV into other modules, and write TyV::Int instead of ty::TyInt. Not sure if that's worthwhile.
@nrc
nrc
commented
Jun 12, 2015

Copy link


Copy Markdown

Member
In general, I don't mind long names so much. TyRef is fine. I think TyArray would be fine instead of TyArrayOrSlice - it is close enough. Dropping the Ty prefix in favour of Ty:: would be nice. But I don't want to see TyV at all - that's indecipherable. (In general, we have a history of short and somewhat bad names in the compiler and we've been moving towards longer, more understandable names. Obviously there are specific places where concision is still warranted though).
nrc
nrc
reviewed
Jun 12, 2015
 View reviewed changes

Comment threadsrc/librustc/middle/ty.rs
@nrcnrcJun 12, 2015

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

maybe Types rather than TyV or TypeVariants?
@eddybeddybJun 12, 2015

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think @nikomatsakis also mentioned TyKind a long time ago (when I considered making this cleanup).
@nrc
nrc
commented
Jun 12, 2015

Copy link


Copy Markdown

Member
This is a great PR, I've wanted to rename these things for a while. Thanks for making the changes. If you could at least change TyArrayOrSlice and TyReference we could land this. You could try changing to use the prefix, or we could land this and do that as a follow up - I realise this PR will rot quite quickly.
@eefriedman
eefriedmanforce-pushed
the
rename-sty
branch
2 times, most recently
from
877fef6 to
9fad9afCompareJune 12, 2015 04:40
@eefriedman
eefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

ContributorAuthor
Rebased, and amended to use TyRef and TyArray.
I think I'll experiment with the prefixes later; there's already a variant named "Types" in librustc/middle/infer/mod.rs.
@nrc
nrc
commented
Jun 12, 2015

Copy link


Copy Markdown

Member
@bors: r+ p=1
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
📌 Commit 9fad9af has been approved by nrc
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
⌛ Testing commit 9fad9af with merge 7626e9f...
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
💔 Test failed - auto-linux-64-nopt-t
@eefriedman
eefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

ContributorAuthor
I didn't rebase correctly; sorry about the churn. Amended with fix.
@nrc
nrc
commented
Jun 12, 2015

Copy link


Copy Markdown

Member
@bors: r+ p=1
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
📌 Commit 00f86fe has been approved by nrc
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
⌛ Testing commit 00f86fe with merge f4bdebf...
bors added a commit
that referenced
this pull request
Jun 12, 2015
@bors
Auto merge of#26232- eefriedman:rename-sty, r=nrc
f4bdebf
Use camel-case naming, and use names which actually make sense in modern Rust.
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
💔 Test failed - auto-mac-64-opt
@eefriedman
Cleanup: rename middle::ty::sty and its variants.
3c69db4
Use camel-case naming, and use names which actually make sense in modern Rust.
@eefriedman
eefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

ContributorAuthor
Updated again; I thought I had built everything, but I somehow missed librustdoc.
@eddyb
eddyb
commented
Jun 12, 2015

Copy link


Copy Markdown

Contributor
@bors r=nrc
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
📌 Commit 3c69db4 has been approved by nrc
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
⌛ Testing commit 3c69db4 with merge 85b5338...
bors added a commit
that referenced
this pull request
Jun 12, 2015
@bors
Auto merge of#26232- eefriedman:rename-sty, r=nrc
85b5338
Use camel-case naming, and use names which actually make sense in modern Rust.
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt
@bors
bors
merged commit 3c69db4
into
rust-lang:masterJun 12, 2015

This was referenced Jun 12, 2015
 Closed

 Merged

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

Reviewers


No reviews


Assignees

@nrcnrc

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@eefriedman@rust-highfive@bors@eddyb@nrc

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Cleanup: rename middle::ty::sty and its variants. - #26232
Merged
bors merged 1 commit into
rust-lang:masterfrom 
eefriedman:rename-sty
Jun 12, 2015
Merged
Cleanup: rename middle::ty::sty and its variants.#26232
bors merged 1 commit into
rust-lang:masterfrom 
eefriedman:rename-sty
Conversation
@eefriedman
@eefriedmaneefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

Contributor
Use camel-case naming, and use names which actually make sense in modern Rust.
@rust-highfive
rust-highfive
commented
Jun 12, 2015

Copy link


Copy Markdown

Contributor
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nrc (or someone else) soon.
If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.
Please see the contribution instructions for more information.
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
☔ The latest upstream changes (presumably #26225) made this pull request unmergeable. Please resolve the merge conflicts.
@eddyb
eddyb
commented
Jun 12, 2015

Copy link


Copy Markdown

Contributor
LGTM, but a could of names seem somewhat long: TyReference and TyArrayOrSlice.

Another thought is that the Ty prefix could be dropped ifTy::Foo could be made to work instead (right now it's not looking so great).
@eefriedman
eefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

ContributorAuthor
I can shorten TyReference to TyRef without much loss of clarity.
I don't really know how to shorten TyArrayOrSlice; TyVec like master uses is misleading, neither TySquare or TyAOrS is obvious, and I can't think of anything else. I'm thinking it will get split into TyArray and TySlice in a followup, so maybe just leave it for now?
As for getting rid of the Ty prefix, it might be possible to rename TypeVariants to something shorter like TyV, sprinkle some use rustc::middle::ty::TyV into other modules, and write TyV::Int instead of ty::TyInt. Not sure if that's worthwhile.
@nrc
nrc
commented
Jun 12, 2015

Copy link


Copy Markdown

Member
In general, I don't mind long names so much. TyRef is fine. I think TyArray would be fine instead of TyArrayOrSlice - it is close enough. Dropping the Ty prefix in favour of Ty:: would be nice. But I don't want to see TyV at all - that's indecipherable. (In general, we have a history of short and somewhat bad names in the compiler and we've been moving towards longer, more understandable names. Obviously there are specific places where concision is still warranted though).
nrc
nrc
reviewed
Jun 12, 2015
 View reviewed changes

Comment threadsrc/librustc/middle/ty.rs
@nrcnrcJun 12, 2015

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

maybe Types rather than TyV or TypeVariants?
@eddybeddybJun 12, 2015

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think @nikomatsakis also mentioned TyKind a long time ago (when I considered making this cleanup).
@nrc
nrc
commented
Jun 12, 2015

Copy link


Copy Markdown

Member
This is a great PR, I've wanted to rename these things for a while. Thanks for making the changes. If you could at least change TyArrayOrSlice and TyReference we could land this. You could try changing to use the prefix, or we could land this and do that as a follow up - I realise this PR will rot quite quickly.
@eefriedman
eefriedmanforce-pushed
the
rename-sty
branch
2 times, most recently
from
877fef6 to
9fad9afCompareJune 12, 2015 04:40
@eefriedman
eefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

ContributorAuthor
Rebased, and amended to use TyRef and TyArray.
I think I'll experiment with the prefixes later; there's already a variant named "Types" in librustc/middle/infer/mod.rs.
@nrc
nrc
commented
Jun 12, 2015

Copy link


Copy Markdown

Member
@bors: r+ p=1
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
📌 Commit 9fad9af has been approved by nrc
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
⌛ Testing commit 9fad9af with merge 7626e9f...
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
💔 Test failed - auto-linux-64-nopt-t
@eefriedman
eefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

ContributorAuthor
I didn't rebase correctly; sorry about the churn. Amended with fix.
@nrc
nrc
commented
Jun 12, 2015

Copy link


Copy Markdown

Member
@bors: r+ p=1
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
📌 Commit 00f86fe has been approved by nrc
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
⌛ Testing commit 00f86fe with merge f4bdebf...
bors added a commit
that referenced
this pull request
Jun 12, 2015
@bors
Auto merge of#26232- eefriedman:rename-sty, r=nrc
f4bdebf
Use camel-case naming, and use names which actually make sense in modern Rust.
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
💔 Test failed - auto-mac-64-opt
@eefriedman
Cleanup: rename middle::ty::sty and its variants.
3c69db4
Use camel-case naming, and use names which actually make sense in modern Rust.
@eefriedman
eefriedman
commented
Jun 12, 2015

Copy link


Copy Markdown

ContributorAuthor
Updated again; I thought I had built everything, but I somehow missed librustdoc.
@eddyb
eddyb
commented
Jun 12, 2015

Copy link


Copy Markdown

Contributor
@bors r=nrc
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
📌 Commit 3c69db4 has been approved by nrc
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
⌛ Testing commit 3c69db4 with merge 85b5338...
bors added a commit
that referenced
this pull request
Jun 12, 2015
@bors
Auto merge of#26232- eefriedman:rename-sty, r=nrc
85b5338
Use camel-case naming, and use names which actually make sense in modern Rust.
@bors
bors
commented
Jun 12, 2015

Copy link


Copy Markdown

Collaborator
☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt
@bors
bors
merged commit 3c69db4
into
rust-lang:masterJun 12, 2015

This was referenced Jun 12, 2015
 Closed

 Merged

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

Reviewers


No reviews


Assignees

@nrcnrc

Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

5 participants

@eefriedman@rust-highfive@bors@eddyb@nrc