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 name-discarding LLVM context #47220

Merged
merged 1 commit into from Jan 6, 2018

Conversation

Projects
None yet
10 participants
@nagisa
Contributor

nagisa commented Jan 5, 2018

This is only applicable when neither of --emit=llvm-ir or --emit=llvm-bc are not
requested.

In case either of these outputs are wanted, but the benefits of such context are
desired as well, -Zfewer_names option provides the same functionality regardless
of the outputs requested.

Should be a viable fix for #46449

Use name-discarding LLVM context
This is only applicable when neither of --emit=llvm-ir or --emit=llvm-bc are not
requested.

In case either of these outputs are wanted, but the benefits of such context are
desired as well, -Zfewer_names option provides the same functionality regardless
of the outputs requested.
@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Jan 5, 2018

Collaborator

r? @petrochenkov

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

Collaborator

rust-highfive commented Jan 5, 2018

r? @petrochenkov

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

@nagisa

This comment has been minimized.

Show comment
Hide comment
@nagisa
Contributor

nagisa commented Jan 5, 2018

@rust-highfive rust-highfive assigned eddyb and unassigned petrochenkov Jan 5, 2018

@rkruppe

This comment has been minimized.

Show comment
Hide comment
@rkruppe

rkruppe Jan 5, 2018

Contributor

Some sort of test would be nice, but the only variant that can reasonably be tested is --emit llvm-ir -Zfewer-names, and even then a plain codegen test would somewhat fragile. Whatever.

@bors r+

Contributor

rkruppe commented Jan 5, 2018

Some sort of test would be nice, but the only variant that can reasonably be tested is --emit llvm-ir -Zfewer-names, and even then a plain codegen test would somewhat fragile. Whatever.

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 5, 2018

Contributor

📌 Commit b719578 has been approved by rkruppe

Contributor

bors commented Jan 5, 2018

📌 Commit b719578 has been approved by rkruppe

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 5, 2018

Contributor

💡 This pull request was already approved, no need to approve it again.

  • There's another pull request that is currently being tested, blocking this pull request: #47156
Contributor

bors commented Jan 5, 2018

💡 This pull request was already approved, no need to approve it again.

  • There's another pull request that is currently being tested, blocking this pull request: #47156
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jan 5, 2018

Contributor

📌 Commit b719578 has been approved by rkruppe

Contributor

bors commented Jan 5, 2018

📌 Commit b719578 has been approved by rkruppe

free((void *)LastError);
LastError = strdup(Err);
}
extern "C" LLVMContextRef LLVMRustContextCreate(bool shouldDiscardNames) {
auto ctx = new LLVMContext();
ctx->setDiscardValueNames(shouldDiscardNames);

This comment has been minimized.

@eddyb

eddyb Jan 5, 2018

Member

Why not expose just that method? Seems self-documenting.

@eddyb

eddyb Jan 5, 2018

Member

Why not expose just that method? Seems self-documenting.

This comment has been minimized.

@nagisa

nagisa Jan 5, 2018

Contributor

Just an arbitrary decision. I feel that function with an argument is slightly cleaner for our use-case.

@nagisa

nagisa Jan 5, 2018

Contributor

Just an arbitrary decision. I feel that function with an argument is slightly cleaner for our use-case.

@eddyb eddyb added the beta-nominated label Jan 5, 2018

kennytm added a commit to kennytm/rust that referenced this pull request Jan 6, 2018

Rollup merge of rust-lang#47220 - nagisa:nonamellvm, r=rkruppe
Use name-discarding LLVM context

This is only applicable when neither of --emit=llvm-ir or --emit=llvm-bc are not
requested.

In case either of these outputs are wanted, but the benefits of such context are
desired as well, -Zfewer_names option provides the same functionality regardless
of the outputs requested.

Should be a viable fix for rust-lang#46449

@kennytm kennytm referenced this pull request Jan 6, 2018

Merged

Rollup of 7 pull requests #47235

bors added a commit that referenced this pull request Jan 6, 2018

Auto merge of #47235 - kennytm:rollup, r=kennytm
Rollup of 7 pull requests

- Successful merges: #46947, #47170, #47190, #47205, #47217, #47220, #47230
- Failed merges: #47233

@bors bors merged commit b719578 into rust-lang:master Jan 6, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nagisa nagisa added the T-compiler label Jan 7, 2018

@michaelwoerister

This comment has been minimized.

Show comment
Hide comment
@michaelwoerister

michaelwoerister Jan 18, 2018

Contributor

I'll prepare a backport of this soon.

Contributor

michaelwoerister commented Jan 18, 2018

I'll prepare a backport of this soon.

bors added a commit that referenced this pull request Jan 18, 2018

Auto merge of #47553 - alexcrichton:beta-next, r=alexcrichton
[beta] Rollup + backports

This is a rollup of two PRs:

* #47546
* #47431

and a backport of two PRs:

* #47505
* #47220
@michaelwoerister

This comment has been minimized.

Show comment
Hide comment
@michaelwoerister

michaelwoerister Jan 19, 2018

Contributor

Seems like @alexcrichton beat me to it :)

Contributor

michaelwoerister commented Jan 19, 2018

Seems like @alexcrichton beat me to it :)

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