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

use field init shorthand in src/librustc/ #43008

Merged
merged 1 commit into from
Jul 6, 2017

Conversation

zackmdavis
Copy link
Member

@zackmdavis zackmdavis commented Jul 1, 2017

Commentary on #37340 suggested using the new field init syntax in the compiler. Do we care about this? If so, here's a pull request for the librustc/ directory. While rustfmt might do this in the future, in the meantime, some simple Python will do:

#!/usr/bin/env python3

import os, re, sys

OPPORTUNITY = re.compile(r" (\w+): \1,?\n")

def field_init_shorthand_substitution(filename):
    with open(filename) as f:
        text = f.read()
        revised = OPPORTUNITY.sub(r" \1,\n", text)
    with open(filename, 'w') as f:
        f.write(revised)

def substitute_in_directory(path):
    for dirname, _subdirs, basenames in os.walk(path):
        for basename in basenames:
            field_init_shorthand_substitution(os.path.join(dirname, basename))

if __name__ == "__main__":
    substitute_in_directory(sys.argv[1])

Update 3 July: edited the search (respectively replace) regex to (\w+): \1,?\n ( \1,\n) from (\w+): \1, ( \1,)

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

@zackmdavis
Copy link
Member Author

(haven't built this locally, it being convenient to just let our friend Travis decide if this actually works)

@@ -36,7 +36,7 @@ impl DepGraphQuery {
}

DepGraphQuery {
graph: graph,
graph,
indices: indices
Copy link
Contributor

Choose a reason for hiding this comment

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

This can also be shortened.

Copy link
Member Author

Choose a reason for hiding this comment

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

hm, yes, regex should've probably been (\w+): \1,?\n rather than (\w+): \1,

@Mark-Simulacrum
Copy link
Member

Is there a reason this is limited to just librustc? It seems appropriate to either run this everywhere or nowhere.

@Mark-Simulacrum Mark-Simulacrum added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 2, 2017
@zackmdavis
Copy link
Member Author

Is there a reason this is limited to just librustc?

It seemed less "dramatic", and less likely to collide with other PRs? Maybe that's not very compelling.

@Mark-Simulacrum
Copy link
Member

I'm happy with starting with just librustc and then expanding to others.

@zackmdavis zackmdavis force-pushed the field_init_shorthand_in_librustc branch from 377425d to 83d5fbf Compare July 3, 2017 18:28
@zackmdavis
Copy link
Member Author

(Travis passed on the initial submission; I've added a newline to the regex (to catch cases like the one Tim points out), reapplied (on master), and force-pushed.)

@PlasmaPower
Copy link
Contributor

Quick shell command to do the same thing as the Python version:

find -type f -print0 | xargs -0 sed -E -i 's/ (\w+): *\1,?$/ \1,/'

If you want to specify a directory, put it right after the find.

@estebank
Copy link
Contributor

estebank commented Jul 5, 2017

@bors try

@bors
Copy link
Contributor

bors commented Jul 5, 2017

⌛ Trying commit 83d5fbf with merge 3c86755...

bors added a commit that referenced this pull request Jul 5, 2017
…=<try>

use field init shorthand in src/librustc/

Commentary on #37340 [suggested](#37340 (comment)) using the new field init syntax in the compiler. Do we care about this? If so, here's a pull request for the librustc/ directory. While [`rustfmt` might do this in the future](#37340 (comment)), in the meantime, some simple Python will do:

```python
#!/usr/bin/env python3

import os, re, sys

OPPORTUNITY = re.compile(r" (\w+): \1,?\n")

def field_init_shorthand_substitution(filename):
    with open(filename) as f:
        text = f.read()
        revised = OPPORTUNITY.sub(r" \1,\n", text)
    with open(filename, 'w') as f:
        f.write(revised)

def substitute_in_directory(path):
    for dirname, _subdirs, basenames in os.walk(path):
        for basename in basenames:
            field_init_shorthand_substitution(os.path.join(dirname, basename))

if __name__ == "__main__":
    substitute_in_directory(sys.argv[1])
```

**Update 3 July**: edited the search (respectively replace) regex to ` (\w+): \1,?\n` (` \1,\n`) from ` (\w+): \1,` (` \1,`)
@bors
Copy link
Contributor

bors commented Jul 6, 2017

☀️ Test successful - status-travis
State: approved= try=True

@estebank
Copy link
Contributor

estebank commented Jul 6, 2017

@bors r+

@bors
Copy link
Contributor

bors commented Jul 6, 2017

📌 Commit 83d5fbf has been approved by estebank

@Mark-Simulacrum
Copy link
Member

For the record, try builds should not be used except to obtain artifacts for cargobomb -- they don't actually test much more than the standard PR builder (just dist, I think).

@estebank
Copy link
Contributor

estebank commented Jul 6, 2017

@Mark-Simulacrum thanks for the clarification! Going forward, should I just r+ and let travis catch any issues then?

@alexcrichton
Copy link
Member

@bors: retry

1 similar comment
@alexcrichton
Copy link
Member

@bors: retry

@bors
Copy link
Contributor

bors commented Jul 6, 2017

🔒 Merge conflict

The field init shorthand syntax was stabilized in 1.17.0 (aebd94f); we
are now free to use it in the compiler.
@zackmdavis zackmdavis force-pushed the field_init_shorthand_in_librustc branch from 83d5fbf to f668999 Compare July 6, 2017 05:45
@zackmdavis
Copy link
Member Author

I don't understand why Git seems to think master's deletion of the libcompiler_builtins directory is a merge conflict?

$ git log --oneline | head -1
8cab2c7 Auto merge of #42899 - alexcrichton:compiler-builtins, r=nikomatsakis
zmd@ExpectedReturn:~/Code/rust$ git merge 83d5fbf
Auto-merging src/librustc/session/mod.rs
Auto-merging src/librustc/middle/stability.rs
Auto-merging src/librustc/middle/dead.rs
CONFLICT (file/directory): There is a directory with name src/libcompiler_builtins in 83d5fbf. Adding src/libcompiler_builtins as src/libcompiler_builtins~HEAD
Automatic merge failed; fix conflicts and then commit the result.

Anyway, rebased and force-pushed.

@estebank
Copy link
Contributor

estebank commented Jul 6, 2017

@bors r+

@bors
Copy link
Contributor

bors commented Jul 6, 2017

📌 Commit f668999 has been approved by estebank

@bors
Copy link
Contributor

bors commented Jul 6, 2017

⌛ Testing commit f668999 with merge afb853a...

bors added a commit that referenced this pull request Jul 6, 2017
…=estebank

use field init shorthand in src/librustc/

Commentary on #37340 [suggested](#37340 (comment)) using the new field init syntax in the compiler. Do we care about this? If so, here's a pull request for the librustc/ directory. While [`rustfmt` might do this in the future](#37340 (comment)), in the meantime, some simple Python will do:

```python
#!/usr/bin/env python3

import os, re, sys

OPPORTUNITY = re.compile(r" (\w+): \1,?\n")

def field_init_shorthand_substitution(filename):
    with open(filename) as f:
        text = f.read()
        revised = OPPORTUNITY.sub(r" \1,\n", text)
    with open(filename, 'w') as f:
        f.write(revised)

def substitute_in_directory(path):
    for dirname, _subdirs, basenames in os.walk(path):
        for basename in basenames:
            field_init_shorthand_substitution(os.path.join(dirname, basename))

if __name__ == "__main__":
    substitute_in_directory(sys.argv[1])
```

**Update 3 July**: edited the search (respectively replace) regex to ` (\w+): \1,?\n` (` \1,\n`) from ` (\w+): \1,` (` \1,`)
@bors
Copy link
Contributor

bors commented Jul 6, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: estebank
Pushing afb853a to master...

@bors bors merged commit f668999 into rust-lang:master Jul 6, 2017
zackmdavis added a commit to zackmdavis/rust that referenced this pull request Aug 15, 2017
Like rust-lang#43008 (f668999), but _much more aggressive_.
bors added a commit that referenced this pull request Aug 16, 2017
…Mark-Simulacrum

use field init shorthand EVERYWHERE

Like #43008 (f668999), but [(lacking reasons to be more timid)](#43008 (comment)) _much more aggressive_.

r? @Mark-Simulacrum
@zackmdavis zackmdavis deleted the field_init_shorthand_in_librustc branch January 13, 2018 07:43
spikespaz pushed a commit to spikespaz/dotwalk-rs that referenced this pull request Aug 29, 2024
…Mark-Simulacrum

use field init shorthand EVERYWHERE

Like #43008 (f668999), but [(lacking reasons to be more timid)](rust-lang/rust#43008 (comment)) _much more aggressive_.

r? @Mark-Simulacrum
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants