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.

Sign up for GitHub

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

Jump to bottom

Remove unnecessary dummy span checks #56221

Merged
merged 4 commits into from
Jan 22, 2019
Merged

Remove unnecessary dummy span checks #56221

merged 4 commits into from
Jan 22, 2019

Conversation

estebank
Copy link
Contributor

The emitter already verifies wether a given span note or span label
can be emitted to the output. If it can't, because it is a dummy
span, it will be either elided for labels or emitted as an unspanned
note/help when applicable.

@rust-highfive
Copy link
Collaborator

r? @varkor

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 25, 2018
@estebank
Copy link
Contributor Author

CC #55669

@estebank estebank changed the title Remove unnecessary dummy span checks WIP Remove unnecessary dummy span checks Nov 25, 2018
@rust-highfive

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@TimNN
Copy link
Contributor

TimNN commented Dec 4, 2018

Ping from triage @varkor: This PR requires your review.

Ping form triage @estebank: It looks like your PR failed on travis and needs a rebase.

@varkor
Copy link
Member

varkor commented Dec 5, 2018

Sorry, I was putting off looking at this because I saw it was marked as "WIP". Is this ready to review now?

@estebank
Copy link
Contributor Author

estebank commented Dec 5, 2018

I'll come back to this soon. I need to add some code to special case the emitter so the notes appear indented instead as left aligned when span == DUMMY_SP.

@varkor varkor added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 1, 2019
@stokhos
Copy link

stokhos commented Jan 14, 2019

Ping from triage @estebank Have you been able to make any progress on this

@estebank
Remove unnecessary dummy span checks
c4b8df5 
The emitter already verifies wether a given span note or span label
can be emitted to the output. If it can't, because it is a dummy
span, it will be either elided for labels or emitted as an unspanned
note/help when applicable.
@estebank estebank force-pushed the remove-dummy-checks branch from 0323d7c to c4b8df5 Compare January 20, 2019 21:29
@estebank estebank changed the title WIP Remove unnecessary dummy span checks Remove unnecessary dummy span checks Jan 20, 2019
@estebank
Copy link
Contributor Author

@stokhos completed

@varkor ready for review

@estebank estebank added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 20, 2019
varkor
varkor reviewed Jan 21, 2019
View reviewed changes
src/libsyntax_pos/lib.rs Outdated Show resolved Hide resolved
varkor
varkor reviewed Jan 21, 2019
View reviewed changes
src/libsyntax_pos/lib.rs Outdated Show resolved Hide resolved
varkor
varkor approved these changes Jan 21, 2019
View reviewed changes
Copy link
Member

@varkor varkor left a comment

Choose a reason for hiding this comment

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

Looks good to me!

varkor and others added 2 commits January 21, 2019 15:53
@varkor @estebank
Fix typo
1d6e5e7 
Co-Authored-By: estebank <[email protected]>
@varkor @estebank
Fix typo
f077990 
Co-Authored-By: estebank <[email protected]>
@estebank
Copy link
Contributor Author

@bors r=varkor

@bors
Copy link
Contributor

bors commented Jan 21, 2019

📌 Commit f077990 has been approved by varkor

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 21, 2019
@bors
Copy link
Contributor

bors commented Jan 22, 2019

⌛ Testing commit f077990 with merge 72754e875b5e5ce1b16709e3621b5547432dc5ab...

@bors
Copy link
Contributor

bors commented Jan 22, 2019

💔 Test failed - checks-travis

@rust-highfive
Copy link
Collaborator

The job x86_64-apple of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log. 
travis_fold:start:before_install.1
travis_time:start:267d892f
$ pip install --user awscli; export PATH=$PATH:$HOME/.local/bin:$HOME/Library/Python/2.7/bin/
Collecting awscli
  Downloading https://files.pythonhosted.org/packages/04/be/6529423f80959e3c9ce05d1425e4237e3c3a01bb4b8a83061ae57ddfa5ab/awscli-1.16.93-py2.py3-none-any.whl (1.4MB)
  Downloading https://files.pythonhosted.org/packages/50/09/c53398e0005b11f7ffb27b7aa720c617aba53be4fb4f4f3f06b9b5c60f28/docutils-0.14-py2-none-any.whl (543kB)
Collecting rsa<=3.5.0,>=3.1.2 (from awscli)
  Downloading https://files.pythonhosted.org/packages/e1/ae/baedc9cb175552e95f3395c43055a6a5e125ae4d48a1d7a924baca83e92e/rsa-3.4.2-py2.py3-none-any.whl (46kB)
Collecting colorama<=0.3.9,>=0.2.5 (from awscli)
---
[00:03:22]    Compiling serde_derive v1.0.81
[00:03:24]    Compiling toml v0.4.10
[00:03:24]    Compiling serde_json v1.0.33
[00:03:35]    Compiling bootstrap v0.0.0 (/Users/travis/build/rust-lang/rust/src/bootstrap)
No output has been received in the last 30m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 22, 2019
@varkor
Copy link
Member

varkor commented Jan 22, 2019

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 22, 2019
@bors
Copy link
Contributor

bors commented Jan 22, 2019

⌛ Testing commit f077990 with merge 76c87a1...

bors added a commit that referenced this pull request Jan 22, 2019
@bors
Auto merge of #56221 - estebank:remove-dummy-checks, r=varkor
76c87a1 
Remove unnecessary dummy span checks

The emitter already verifies wether a given span note or span label
can be emitted to the output. If it can't, because it is a dummy
span, it will be either elided for labels or emitted as an unspanned
note/help when applicable.
@bors
Copy link
Contributor

bors commented Jan 22, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: varkor
Pushing 76c87a1 to master...

@bors bors merged commit f077990 into rust-lang:master Jan 22, 2019
@kennytm kennytm mentioned this pull request Feb 22, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

@varkor varkor varkor approved these changes

Assignees

@varkor varkor

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@estebank @rust-highfive @bors @TimNN @varkor @stokhos @petrochenkov