Skip to content
This repository has been archived by the owner on Feb 14, 2021. It is now read-only.

Fix for issue #82 - duplicate symbol error: memset, memcpy, memove an… #83

Merged
merged 2 commits into from
Feb 6, 2019
Merged

Fix for issue #82 - duplicate symbol error: memset, memcpy, memove an… #83

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8cb0831
Fix for issue #82 - duplicate symbol error: memset, memcpy, memove an…
dforsten Feb 5, 2019
cf0e939
Reverted removal of entire pwasm_libc, removed rlibc from it instead
dforsten Feb 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion libc/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ keywords = ["wasm", "parity", "webassembly", "blockchain"]
categories = ["no-std", "embedded"]

[dependencies]
rlibc = "1.0"
cfg-if = "0.1"

[features]
Expand Down
2 changes: 0 additions & 2 deletions libc/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,5 @@ cfg_if! {
pub unsafe extern "C" fn memset(dest: *mut u8, c: i32, n: usize) -> *mut u8 {
ext_memset(dest, c, n)
}
} else {
extern crate rlibc;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think only this line should be removed, we'd better keep external memory ops for libc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried it locally and that single-line change alone fixes the link issue as well.
Should I make a new PR, or commit a revert?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, you can update this PR since it's not merged :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense.
I think it would make sense to move this code into the top-level crate also (as a separate mod, but not a crate). I don't see any reason in the current granularity. Have a separated pwasm-libc crate for the purpose it serves currently has not too much sense to me.

Copy link
Contributor

@pepyakin pepyakin Feb 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, yeah, it is a special feature, then we can probably comment this line indeed.

However, I'm having a hard time to understand/remember why would we need this feature in the first place. Substrate doesn't use these, nor these ext_* functions are provided by the pwasm runtime.

(That said, I don't have any objections from merging this as is)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change committed and passed the tests!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussions about refactoring the code should probably be done outside of this immediate bug fix PR. Right now I am just very eager to start multiplying U256 values in my pwasm contracts 😜.

}
}