Skip to content

Add testcase for issue-32948#36832

Merged
bors merged 1 commit into
rust-lang:masterfrom
ParkHanbum:master
Oct 6, 2016
Merged

Add testcase for issue-32948#36832
bors merged 1 commit into
rust-lang:masterfrom
ParkHanbum:master

Conversation

@ParkHanbum

@ParkHanbum ParkHanbum commented Sep 29, 2016

Copy link
Copy Markdown
Contributor

#32948

issue-32948 is similar to issue-32554.

issue-32948 : Symbol names for monomorphized trait impls are not stable across crates
issue-32554 : Symbol names for generics are not stable across crates

so, I append issue-32948's testcase to issue-32554's testcase.
thanks!

@rust-highfive

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 @nikomatsakis (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. Due to 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.

@nikomatsakis

Copy link
Copy Markdown
Contributor

r? @michaelwoerister

@michaelwoerister

Copy link
Copy Markdown
Member

Thanks a lot, @ParkHanbum!
This sed script is getting a bit unreadable (due to no fault of yours). Can you replace it with the following?:

# The following command will:
#  1. dump the symbols of a library using `nm`
#  2. extract only those lines that we are interested in via `grep`
#  3. from those lines, extract just the symbol name via `sed`
#     (symbol names always start with "_ZN" and end with "E")
#  4. sort those symbol names for deterministic comparison
#  5. write the result into a file

dump-symbols = nm "$(TMPDIR)/lib$(1).rlib" \
             | grep -E "some_test_function|Bar|bar" \
             | sed "s/.*\(_ZN.*E\).*/\1/" \
             | sort \
             > "$(TMPDIR)/$(1).nm"

That would make it easier for future maintainers. If you do that, I'd be happy to approve the PR.

@ParkHanbum

Copy link
Copy Markdown
Contributor Author

@michaelwoerister cosider it done!! I'll be better then this PR next. thanks a lot !!

@michaelwoerister

Copy link
Copy Markdown
Member

@bors r+

@bors

bors commented Oct 6, 2016

Copy link
Copy Markdown
Collaborator

📌 Commit cb700e7 has been approved by michaelwoerister

@michaelwoerister

Copy link
Copy Markdown
Member

Thanks a lot, @ParkHanbum! My comment wasn't meant as criticism. sed scripts are just hard to decipher :)

@michaelwoerister

Copy link
Copy Markdown
Member

@bors p=rollup

Let's see if that works...

@michaelwoerister

Copy link
Copy Markdown
Member

@bors rollup

sophiajt pushed a commit to sophiajt/rust that referenced this pull request Oct 6, 2016
Add testcase for issue-32948

issue-32948 is similar to issue-32554.

issue-32948 : Symbol names for monomorphized trait impls are not stable across crates
issue-32554 : Symbol names for generics are not stable across crates

so, I append issue-32948's testcase to issue-32554's testcase.
thanks!
bors added a commit that referenced this pull request Oct 6, 2016
@bors bors merged commit cb700e7 into rust-lang:master Oct 6, 2016
@michaelwoerister

Copy link
Copy Markdown
Member

🎉

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants