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

Replaced constant fixed-length u8 vectors with byte strings (fixes #3257) #3324

Closed
wants to merge 4 commits into from

Conversation

@thomas-daniels
Copy link
Contributor

thomas-daniels commented Sep 13, 2014

Replaced constant fixed-length u8 vectors in CodegenRust.py with byte strings (fixes #3257).

@jdm said that there is also an occurrence of a fixed-length vector in eventtarget.rs. I found this one, but when I tried to replace it, it gave errors (because it is not a u8 vector but a c_char vector). When I replaced it with b"...", it said:

dom/eventtarget.rs:182:46: 182:56 error: mismatched types: expected `&'static [i8]` but found `&'static [u8]` (expected i8 but found u8)

And when I did the same but with dropping the b (so, by changing it to "..."), it said:

dom/eventtarget.rs:182:46: 182:55 error: mismatched types: expected `&'static [i8]` but found `&'static str` (expected vector but found str)

I don't know whether there are other prefixes for the strings to make this work, but if there are, let me know and I'll use it there.

@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Sep 13, 2014

Critic review: https://critic.hoppipolla.co.uk/r/2586

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@highfive
Copy link

highfive commented Sep 13, 2014

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @larsbergstrom (or someone else) soon.

@thomas-daniels
Copy link
Contributor Author

thomas-daniels commented Sep 13, 2014

Just saw that the Travis CI build failed. Pushed a new commit to fix this.

@Manishearth
Copy link
Member

Manishearth commented Sep 13, 2014

Nice to see you taking interest in Servo! :D

@thomas-daniels
Copy link
Contributor Author

thomas-daniels commented Sep 13, 2014

I put the parenthesis to fix the Travis CI build error at the wrong place; pushed a new commit to fix this.

@jdm
Copy link
Member

jdm commented Sep 13, 2014

…string in eventtarget.rs
@thomas-daniels
Copy link
Contributor Author

thomas-daniels commented Sep 13, 2014

With the help of Ms2ger and huon on IRC, I found a way to make the byte strings in eventtarget.rs work. I also fixed the latest Travis build error. Because the codegen apparently doesn't run automatically (and I have no idea how to run in manually), I could not check whether I still have got more errors, so I'll have to wait for the Travis CI build to see that.


decls = ''.join([stringDecl(m) for m in array])
return decls + self.generatePrefableArray(
array, name,
' JSFunctionSpec {name: &%s_name as *const u8 as *const libc::c_char, call: JSNativeWrapper {op: Some(%s), info: %s}, nargs: %s, flags: %s as u16, selfHostedName: 0 as *const libc::c_char }',
' JSFunctionSpec {name: %s_name.as_ptr() as *const u8 as *const libc::c_char, call: JSNativeWrapper {op: Some(%s), info: %s}, nargs: %s, flags: %s as u16, selfHostedName: 0 as *const libc::c_char }',

This comment has been minimized.

@jdm

jdm Sep 14, 2014

Member

Ah yes, this won't work since we can't call functions in constants. Maybe &%s_name[0] instead?

This comment has been minimized.

@huonw

huonw Sep 14, 2014

Contributor

(It may hit rust-lang/rust#17233, depending on how s_name is created.)


decls = ''.join([stringDecl(m) for m in array])

return decls + self.generatePrefableArray(
array, name,
' JSPropertySpec { name: &%s_name as *const u8 as *const libc::c_char, tinyid: 0, flags: ((%s) & 0xFF) as u8, getter: %s, setter: %s }',
' JSPropertySpec { name: %s_name.as_ptr() as *const u8 as *const libc::c_char, tinyid: 0, flags: ((%s) & 0xFF) as u8, getter: %s, setter: %s }',

This comment has been minimized.

@jdm

jdm Sep 14, 2014

Member

Same deal here.

static Class: DOMJSClass = DOMJSClass {
base: js::Class {
name: &Class_name as *const u8 as *const libc::c_char,
name: Class_name.as_ptr() as *const u8 as *const libc::c_char,

This comment has been minimized.

@jdm

jdm Sep 14, 2014

Member

And here.

static PrototypeClass: JSClass = JSClass {
name: &PrototypeClassName__ as *const u8 as *const libc::c_char,
name: PrototypeClassName__.as_ptr() as *const u8 as *const libc::c_char,

This comment has been minimized.

@jdm

jdm Sep 14, 2014

Member

Aaaaand here.

@thomas-daniels
Copy link
Contributor Author

thomas-daniels commented Sep 14, 2014

@jdm I tried &%s_name[0], but that didn't work unfortunately. I got this error:

rustc: /home/larsberg/rust/src/llvm/include/llvm/Support/Casting.h:237: typename llvm::cast_retty<X, Y*>::ret_type llvm::cast(Y*) [with X = llvm::GlobalVariable; Y = llvm::Value; typename llvm::cast_retty<X, Y*>::ret_type = llvm::GlobalVariable*]: Assertion `isa<X>(Val) && "cast<Ty>() argument of incompatible type!"' failed.

I also tried to use functions instead of static variables, and creating a local non-static variable inside that function to perform the casting, but that didn't work; it said that function calls are only allowed in constructors, and I had to use it inside a static class.

@jdm
Copy link
Member

jdm commented Sep 14, 2014

Yeah, I think the LLVM problem is rust-lang/rust#17233, so we might be stuck until that gets fixed.

@thomas-daniels
Copy link
Contributor Author

thomas-daniels commented Sep 14, 2014

Alright, then I'll wait until that's fixed.

@Ms2ger Ms2ger closed this Nov 11, 2014
@Manishearth
Copy link
Member

Manishearth commented Dec 9, 2014

Does this work now?

@thomas-daniels
Copy link
Contributor Author

thomas-daniels commented Dec 23, 2014

@Manishearth The linked issue is still open, so I'd say it doesn't. (I would test it, but I don't know what exactly to test for -- I did many different things to try to make it work)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.