Add support for array-length for parameters and return values #387

sdroege · 2017-06-19T08:15:16Z

Only pointer arrays are currently supported, the translation trait is even called FromGlibPtrContainer. This should be changed really to also be able to handle all kinds of integer (incl. byte) arrays.

sdroege · 2017-06-19T14:47:27Z

With #389 and this, Gtk.RecentInfo can be fully autogenerated btw.

EPashkin · 2017-06-19T16:46:05Z

src/codegen/function_body_chunk.rs

        self.parameters.push(Parameter::Out {
-            parameter: parameter.into(),
+            parameter:  parameter_ffi_call_out::Parameter::new(parameter, array_length),


Extra space

EPashkin · 2017-06-19T16:47:14Z

src/codegen/return_value.rs

@@ -36,12 +36,12 @@ impl ToReturnValue for analysis::return_value::Info {
 pub fn out_parameter_as_return_parts(analysis: &analysis::functions::Info)
                                     -> (&'static str, &'static str) {
    use analysis::out_parameters::Mode::*;
-    let is_tuple = analysis.outs.len() > 1;
+    let num_outs = analysis.outs.iter().filter(|p| p.array_length.is_none()).count();


fn len() now unused

EPashkin · 2017-06-19T16:50:23Z

Bad change in glib:key_file - missed tuple

-    pub fn get_string_list(&self, group_name: &str, key: &str) -> Result<(Vec<String>, usize), Error> {
+    pub fn get_string_list(&self, group_name: &str, key: &str) -> Result<Vec<String>, usize, Error> {

EPashkin · 2017-06-19T16:50:51Z

..and still return usize

EPashkin · 2017-06-19T17:17:50Z

src/codegen/translate_from_glib.rs

    }
 }
+


EPashkin · 2017-06-19T17:23:05Z

Too many long lines but code too.
Maybe code will be simpler is analysis parameter has is_array_len flag.

EPashkin · 2017-06-19T17:26:05Z

or even SkipReason: None/InArrayLen/OutArrayLen

EPashkin · 2017-06-19T18:05:37Z

*Too many long lines but it not big problem as code works.

sdroege · 2017-06-19T18:47:18Z

Thanks, I'll clean this up tomorrow. For the code style, what do you think about my suggestion to just run things through rustfmt soon and then require pull requests to use the correct, automatically enforced format?

EPashkin · 2017-06-19T19:08:00Z

also maybe better pass array_length: Option<(String, String)> directly to builder.

EPashkin · 2017-06-19T19:45:09Z

About rustfmt, currently it change nothing in gir or panic with 'failed to emit error: operation not supported by the terminal'
Maybe it because some cargo bug: it build only if error stream relocated to file.
When run with 2>_ it also fails on build.rs when opening file gir\\src/git.rs (windows related problem).
Result code sometimes looks strange:

pub fn generate(
    w: &mut Write,
    env: &Env,
    prop: &ChildProperty,
    in_trait: bool,
    only_declaration: bool,
    indent: usize,
) -> Result<()> {
    try!(generate_func(
        w,
        env,
        prop,
        in_trait,
        only_declaration,
        indent,
        true,
    ));
    try!(generate_func(
        w,
        env,
        prop,
        in_trait,
        only_declaration,
        indent,
        false,
    ));

    Ok(())
}

So I don't against single apply in separate PR, but IMHO it don't ready to enforce.

For parameters and fields.

Only pointer typed array elements are supported for now. gtk-rs#376

… parameter position

sdroege · 2017-06-20T21:11:45Z

The comments should all be covered now. Remaining problem is g_key_file_set_string_list(), which has the wrong type in the sys crate already. The list should be *const *const c_char but is not detected as such and also looks wrong in the .gir file. Once that is fixed, the ToGlibPtr instance for [&str] from glib/src/translate.rs has to be enabled again.
This seems to be a bug in gobject-introspection, where it fails to parse const gchar * const list[] correctly because of the square brackets probably.

Unrelated, g_strv_contains() is also wrong but not in the sys crate (it has const gchar * const * as type there, no square brackets). It should be &[&str] for the first parameter.

EPashkin · 2017-06-21T03:13:38Z

Gio generation failed, other lib changes seems fine

D:\eap\rust\0\gir>cargo run --release -- -s -c D:\eap\rust\0\gio\Gir.toml
    Finished release [optimized] target(s) in 0.0 secs
     Running `D:\eap\rust\0\./target\release\gir.exe -s -c D:\eap\rust\0\gio\Gir.toml`
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', src\libcore\option.rs:329
stack backtrace:
   0: std::sys_common::backtrace::_print
   1: std::panicking::default_hook::{{closure}}
   2: std::panicking::default_hook
   3: std::panicking::rust_panic_with_hook
   4: std::panicking::begin_panic
   5: std::panicking::begin_panic_fmt
   6: rust_begin_unwind
   7: core::panicking::panic_fmt
   8: core::panicking::panic
   9: gir::codegen::return_value::out_parameters_as_return
  10: gir::codegen::function::declaration
  11: gir::codegen::function::generate
  12: gir::codegen::object::generate
  13: gir::file_saver::save_to_file
  14: gir::codegen::objects::generate
  15: gir::codegen::normal_generate
  16: gir::do_main
  17: gir::main
  18: _rust_maybe_catch_panic
  19: std::rt::lang_start
  20: _tmainCRTStartup
  21: mainCRTStartup
  22: free_fail_stack_return
error: process didn't exit successfully: `D:\eap\rust\0\./target\release\gir.exe -s -c D:\eap\rust\0\gio\Gir.toml` (exit code: 101)

EPashkin · 2017-06-21T03:34:52Z

src/codegen/function.rs

+    let ret_array_length = if let Some(pos) = analysis.ret.parameter.as_ref().and_then(|p| p.array_length) {
+        // The first parameter is &self in case of methods
+        let pos_offset = if analysis.kind == library::FunctionKind::Method { 1 } else { 0 };
+        analysis.parameters.iter().nth((pos + pos_offset) as usize)


clippy warning: called .iter().nth() on a Vec. Calling .get() is both faster and more readable

EPashkin · 2017-06-21T03:35:24Z

src/codegen/function.rs

+                use analysis::out_parameters::Mode;
+                // The actual return value was inserted at position 0
+                let pos_offset = if analysis.outs.mode == Mode::Combined || analysis.outs.mode == Mode::Throws(true) { 1 } else { 0 };
+                analysis.parameters.iter().nth((pos + pos_offset) as usize)


In the outs array they are the original one, in the params array not. It might make sense to use analysis::parameter::Info instead of library::Parameter for the outs array too.

sdroege · 2017-06-21T07:26:04Z

https://bugzilla.gnome.org/show_bug.cgi?id=784020 (glib)
https://bugzilla.gnome.org/show_bug.cgi?id=784021 (pango)
https://bugzilla.gnome.org/show_bug.cgi?id=784022 (gtk)

For string parameters + array length, we currently still generate wrong code it seems. For some reason it is interpreted as a &[&str] then instead of a &str. E.g. in Pango after changing the .gir file for set_markup like this

-            <type name="utf8" c:type="const char*"/>
+            <array length="1" zero-terminated="0" c:type="char*">
+              <type name="utf8" c:type="char"/>
+            </array>

@EPashkin Any idea why, or should I research/debug?

sdroege · 2017-06-21T13:22:24Z

https://bugzilla.gnome.org/show_bug.cgi?id=784035 (bug in gobject-introspection that causes it to drop const)

EPashkin · 2017-06-21T13:43:08Z

<type name="utf8" c:type="char"/> gir think that type "utf8" is string and ignore "char" without * in normal mode.
In sys mode it maybe do different

EPashkin · 2017-06-21T13:46:55Z

One symbol has other type name. IMHO pango_layout_set_markup accepts just text, and len need used from it.

sdroege · 2017-06-21T13:49:31Z

gir think that type "utf8" is string and ignore "char" without * in normal mode.

Ah good point. I guess we need to parse the c:type of this, or the surrounding to know for sure. Otherwise this is ambiguous.

In sys mode it maybe do different

In sys mode it seems to count the asterisks (i.e. look at the c:type instead)

One symbol has other type name. IMHO pango_layout_set_markup accepts just text, and len need used from it.

What do you mean? It also has to be changed the same way as set_text, yes.

EPashkin · 2017-06-21T13:53:51Z

I mean that pango_layout_set_markup accepts not array but utf8 string and array-length attribute may be cant applied to it. Same as "set_text".

sdroege · 2017-06-21T13:57:57Z

They all accept a string, yes. Which is an array of utf8 characters, and the length argument is the number of characters in there. What you basically mean is what I wrote here https://bugzilla.gnome.org/show_bug.cgi?id=784035#c1 ?

In GLib and elsewhere, string parameters with an additional length parameter for the string length already use the (array length) annotation, and conceptually it's correct.

EPashkin · 2017-06-21T16:34:32Z

I meant that string uft8 != array [u8] (at minimum it "implies" that contains valid utf8 characters)
so I don't think that annotations in https://bugzilla.gnome.org/show_bug.cgi?id=784021 can be applied to strings.

sdroege · 2017-06-21T16:38:22Z

All those functions are working with UTF8 strings, not [u8]. The markup ones just in addition require the strings to be valid Pango markup (or things will fail), just like GTK labels if you configure them to show Pango markup

EPashkin · 2017-06-21T16:39:49Z

IMHO all array-related problems is solved, can I merge this, or you want add something?

sdroege · 2017-06-21T16:43:36Z

It's doing the wrong things for strings, I'd like to fix that as part of this. Otherwise the array length annotations on strings (as exist in various libraries already) cause wrong code to be generated.

But you could merge it now already if you want, and I'll work on that in another pull request then.

EPashkin · 2017-06-21T16:53:26Z

Ok, I wait then

sdroege · 2017-06-22T11:13:07Z

Good to merge. I discussed the string+length stuff with gobject-introspection people on IRC, and there's no way to correctly annotate that currently unless your "string" is actually an array of bytes (and not a UTF8 string), in which case it would be (array length=len) (element-type guint8).

EPashkin · 2017-06-22T11:31:38Z

Thanks, @sdroege.
After merge I will wait for regen PRs (glib and gtk most needed as it don't builds).

sdroege · 2017-06-22T11:32:59Z

I do them, you do them, ...?

EPashkin · 2017-06-22T11:33:51Z

I wait

😉

sdroege · 2017-06-22T12:20:14Z

Alright, I'll try to get to that tonight then :)

sdroege · 2017-06-24T08:51:12Z

All submitted

EPashkin · 2017-06-24T09:12:35Z

@sdroege can you add link to all these PRs to this or better to #376 ?

sdroege force-pushed the array-length branch from 8d0fc1e to 87ce1b0 Compare June 19, 2017 10:06

EPashkin reviewed Jun 19, 2017

View reviewed changes

src/codegen/translate_from_glib.rs Outdated

}

}

Copy link

Member

EPashkin Jun 19, 2017

Choose a reason for hiding this comment

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

Extra line

sdroege added 4 commits June 20, 2017 23:17

Parse (array length=X) field from the GIR

aef99e0

For parameters and fields.

Add support for array-length for parameters and return values

d9f893a

Only pointer typed array elements are supported for now. gtk-rs#376

Minor cleanup

89b552f

Calculate array length positions based on parameter position, not out…

bf35750

… parameter position

sdroege force-pushed the array-length branch from 87ce1b0 to bf35750 Compare June 20, 2017 21:06

EPashkin reviewed Jun 21, 2017

View reviewed changes

sdroege added 2 commits June 21, 2017 09:21

Use Vec<_>::get() instead of iter().nth()

5269a93

Mangle parameter names before comparison

017e7cc

In the outs array they are the original one, in the params array not. It might make sense to use analysis::parameter::Info instead of library::Parameter for the outs array too.

This was referenced Jun 22, 2017

Remove length argument from SelectionData::set_text gtk-rs/gtk#477

Closed

Why does Layout::set_text need a length argument? gtk-rs/pango#61

Closed

EPashkin merged commit 2071a93 into gtk-rs:master Jun 22, 2017

Add support for array-length for parameters and return values #387

Add support for array-length for parameters and return values #387

Conversation

sdroege commented Jun 19, 2017

sdroege commented Jun 19, 2017

EPashkin Jun 19, 2017

Choose a reason for hiding this comment

EPashkin Jun 19, 2017

Choose a reason for hiding this comment

EPashkin commented Jun 19, 2017

EPashkin commented Jun 19, 2017 • edited Loading

EPashkin Jun 19, 2017

Choose a reason for hiding this comment

EPashkin commented Jun 19, 2017

EPashkin commented Jun 19, 2017

EPashkin commented Jun 19, 2017

sdroege commented Jun 19, 2017

EPashkin commented Jun 19, 2017

EPashkin commented Jun 19, 2017

sdroege commented Jun 20, 2017 • edited Loading

EPashkin commented Jun 21, 2017

EPashkin Jun 21, 2017

Choose a reason for hiding this comment

EPashkin Jun 21, 2017

Choose a reason for hiding this comment

sdroege commented Jun 21, 2017

sdroege commented Jun 21, 2017

EPashkin commented Jun 21, 2017

EPashkin commented Jun 21, 2017

sdroege commented Jun 21, 2017

EPashkin commented Jun 21, 2017

sdroege commented Jun 21, 2017

EPashkin commented Jun 21, 2017

sdroege commented Jun 21, 2017 • edited Loading

EPashkin commented Jun 21, 2017

sdroege commented Jun 21, 2017

EPashkin commented Jun 21, 2017

sdroege commented Jun 22, 2017

EPashkin commented Jun 22, 2017

sdroege commented Jun 22, 2017

EPashkin commented Jun 22, 2017 • edited Loading

sdroege commented Jun 22, 2017

sdroege commented Jun 24, 2017

EPashkin commented Jun 24, 2017

EPashkin commented Jun 19, 2017 •

edited

Loading

sdroege commented Jun 20, 2017 •

edited

Loading

sdroege commented Jun 21, 2017 •

edited

Loading

EPashkin commented Jun 22, 2017 •

edited

Loading