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

Rename #873

Merged
merged 5 commits into from Nov 20, 2019
Merged

Rename #873

merged 5 commits into from Nov 20, 2019

Conversation

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Nov 19, 2019

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Nov 19, 2019

@@ -514,6 +514,11 @@ fn analyze_function(
.filter_map(|f| f.version)
.min()
.or(func.version);
let name = configured_functions
.iter()
.filter_map(|f| f.rename.clone())

This comment has been minimized.

Copy link
@sdroege

sdroege Nov 19, 2019

Member

Shouldn't you filter here first to make sure f.name == name? Or does that happen elsewhere already?

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Nov 19, 2019

Author Member

Indeed!

This comment has been minimized.

Copy link
@sdroege

sdroege Nov 19, 2019

Member

Does the filter_map just above also miss that? Maybe the filtering is actually happening somewhere else already and that's why it works at all? Check the code in more detail please :)

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Nov 19, 2019

Author Member

I can confirm that it's not good. Fixing it. :)

This comment has been minimized.

Copy link
@EPashkin

EPashkin Nov 19, 2019

Member

configured_functions is filtered by function name by caller,
only problem is it pattern that applied to many functions,
and IMHO its not problem.
I more worry about docs.

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Nov 19, 2019

Author Member

That's a good point.

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Nov 19, 2019

Author Member

I'll need to check for the doc generation. For now, it's very certainly broken in this case.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:rename branch from f688389 to 79f6d28 Nov 19, 2019
@@ -254,8 +254,10 @@ fn create_object_doc(w: &mut dyn Write, env: &Env, info: &analysis::object::Info
};

for function in functions {
let configured_functions = obj.functions.matched(&function.name);

This comment has been minimized.

Copy link
@EPashkin

EPashkin Nov 20, 2019

Member

IMHO function name here replaced already, so no one will be found here

This comment has been minimized.

Copy link
@GuillaumeGomez

GuillaumeGomez Nov 20, 2019

Author Member

You fell in the same trap as I did. :)

Here is a library::Function list, whereas I make the change inside the analysis::functions::Info type. So at this point, function.name is still correct, which is why I need to get the rename thing here.

This comment has been minimized.

Copy link
@EPashkin

EPashkin Nov 20, 2019

Member

You right 😉
Then only need some formating

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:rename branch from 87dd4c5 to 42c8dc4 Nov 20, 2019
@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Nov 20, 2019

Formatting fixed too now.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Nov 20, 2019

@EPashkin EPashkin merged commit f0ecd66 into gtk-rs:master Nov 20, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@GuillaumeGomez GuillaumeGomez deleted the GuillaumeGomez:rename branch Nov 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.