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

Generate code using CStringHolder instead of String #666

Merged
merged 2 commits into from Dec 15, 2018

Conversation

Projects
None yet
2 participants
@philn
Copy link
Contributor

philn commented Nov 18, 2018

Generated code relying on CStringHolder should be more efficient because it
reduces the amount of string copies.

@philn philn force-pushed the philn:master branch from 7e02c5f to d6f71da Nov 25, 2018

@philn philn changed the title WIP: Generate code using CStringHolder instead of String Generate code using CStringHolder instead of String Nov 25, 2018

@philn

This comment has been minimized.

Copy link
Contributor Author

philn commented Nov 25, 2018

Ok I think this is ready for review, @sdroege @GuillaumeGomez @EPashkin

@philn

This comment has been minimized.

Copy link
Contributor Author

philn commented Nov 25, 2018

CI fails on Pango for which I have a branch. Along with most of gtk-rs modules for which bindings need to be regenerated, I have branches for most of those at this point. I can open PRs for them now or after this one + the glib one is merged. Please advise ;)

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Nov 25, 2018

Seems it use need fixed as 'use glib::cstringingholder;`. Just add it to all imports (for glib it will be removed automatically).

error[E0433]: failed to resolve. Use of undeclared type or module `cstringholder`
  --> D:/eap/rust/0/gio\src\auto\settings_schema_source.rs:38:57
   |
38 |     pub fn list_schemas(&self, recursive: bool) -> (Vec<cstringholder::CStringHolder>, Vec<cstringholder::CStringHolder>) {
   |                                                         ^^^^^^^^^^^^^ Use of undeclared type or module `cstringholder`

Also adding it unneeded in pango\src\auto\coverage.rs, atk\src\auto\socket.rs, gio\src\auto\settings_backend.rs, gio\src\auto\subprocess_launcher.rs

@philn

This comment has been minimized.

Copy link
Contributor Author

philn commented Nov 25, 2018

Ah, I didn't test gio with the v2_40 feature indeed, I'll fix it. I wanted to avoid "unused" warnings so I tried to import the module only when it's needed in the generated code.

@philn philn force-pushed the philn:master branch from d6f71da to 7cd252e Nov 25, 2018

@philn

This comment has been minimized.

Copy link
Contributor Author

philn commented Nov 25, 2018

gio-rs regen there, philn/gio@ea028eb

@EPashkin EPashkin referenced this pull request Dec 7, 2018

Merged

lib: Introduce GString #389

@philn philn force-pushed the philn:master branch from 7cd252e to 7a86f17 Dec 9, 2018

@philn

This comment has been minimized.

Copy link
Contributor Author

philn commented Dec 9, 2018

PR updated!

@EPashkin
Copy link
Member

EPashkin left a comment

Thanks for fixing trampolines.
There 10 unused cases in gtk, I look at it later today or tomorrow.

Show resolved Hide resolved src/codegen/mod.rs Outdated
Show resolved Hide resolved src/analysis/rust_type.rs Outdated
Show resolved Hide resolved src/analysis/trampolines.rs Outdated
Show resolved Hide resolved src/codegen/property_body.rs Outdated
Show resolved Hide resolved src/codegen/return_value.rs Outdated
Show resolved Hide resolved src/codegen/return_value.rs Outdated
Show resolved Hide resolved src/library.rs Outdated
@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Dec 9, 2018

@philn After this patch, you can remove all manual imports, at minimum on gtk-rs 😉
https://gist.github.com/EPashkin/4780324a59f986be68028f152ce7cf07

@philn

This comment has been minimized.

Copy link
Contributor Author

philn commented Dec 9, 2018

Awesome! I'll check this tomorrow, thanks!

@philn philn force-pushed the philn:master branch 2 times, most recently from 36b7e86 to 7c7a820 Dec 15, 2018

@philn

This comment has been minimized.

Copy link
Contributor Author

philn commented Dec 15, 2018

PR updated, much nicer thanks to your patch @EPashkin :)

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Dec 15, 2018

@philn Now it really much shorter and without new warning. Thanks.
Can you add this patch as final touch?
It use &GString::from_glib_borrow in trampolines

+++ b/src/analysis/trampoline_parameters.rs
@@ -137,6 +137,7 @@ pub fn analyze(
 
         let conversion_type = {
             match *env.library.type_(par.typ) {
+                library::Type::Fundamental(library::Fundamental::Utf8) |
                 library::Type::Record(..) |
                 library::Type::Interface(..) |
                 library::Type::Class(..) => ConversionType::Borrow,

philn and others added some commits Dec 1, 2018

Generate code using GString instead of String
Generated code relying on GString should be more efficient because it
reduces the amount of Rust String allocations.

@philn philn force-pushed the philn:master branch from 7c7a820 to 71c9b66 Dec 15, 2018

@philn

This comment has been minimized.

Copy link
Contributor Author

philn commented Dec 15, 2018

Done. Thanks for the reviews and help

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Dec 15, 2018

@philn Thanks again, Lets merge to allow update glib's PR.
Don't forget use glib/Gir_GObject.toml too 😉

@EPashkin EPashkin merged commit c5498ac into gtk-rs:master Dec 15, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.