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

handle aliases better #702

Merged
merged 1 commit into from Jan 22, 2019

Conversation

Projects
None yet
3 participants
@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Jan 22, 2019

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jan 22, 2019

IMHO both function need "fundamental" in name and return bool to remove wrong usage

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jan 22, 2019

What names do you suggest then? :)

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jan 22, 2019

Also get_chunk that actually adds chunk IMHO bad name.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jan 22, 2019

get_type_id->is_fundamental
and second is tough, maybe just get->add and comment about returning bool

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:user-callbacks branch from c9078f3 to 396caf7 Jan 22, 2019

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jan 22, 2019

Renamed and updated a bit.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jan 22, 2019

For add_chunk_for_type called env.type_ internally, then call env.type_ again on result IMHO just need bool

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jan 22, 2019

Seems fine to me but maybe this should be part of the is_fundamental() method on the types directly? It is lying currently for alias types :)

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jan 22, 2019

Well, it's an alias so it's lying that much.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jan 22, 2019

Maybe instead first function better add is_fundamental_type to library?

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jan 22, 2019

and remove type returning from add_chunk_for_type and use is_fundamental_type, this IMHO make code cleaner

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jan 22, 2019

That will be better this way indeed. Let's go for that!

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:user-callbacks branch from 396caf7 to 662e715 Jan 22, 2019

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jan 22, 2019

Updated. I decided to create a function in library directly in case we'll need it again in the future.

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jan 22, 2019

Still good for me :)

@@ -779,6 +783,15 @@ impl Type {
}
}

/// If the type is an Alias containing a fundamental, it'll return true (whereas
/// `is_fundamental` won't).
pub fn is_fundamental_type(&self, env: &Env) -> bool {

This comment has been minimized.

@EPashkin

EPashkin Jan 22, 2019

Member

Seems old is_fundamental used only internally so we can rename it to `is_fundamental_internal" (maybe make not public)

This comment has been minimized.

@EPashkin

EPashkin Jan 22, 2019

Member

But IMHO it good as is.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jan 22, 2019

@GuillaumeGomez Thanks, just waiting CI

match *type_ {
library::Type::Fundamental(ref x) if !x.requires_conversion() => true,
library::Type::Fundamental(library::Fundamental::Boolean) |
library::Type::Fundamental(library::Fundamental::UniChar) => {

This comment has been minimized.

@EPashkin

EPashkin Jan 22, 2019

Member

By travis error Utf8 need be false too, maybe with Filename and OsString

This comment has been minimized.

@EPashkin

EPashkin Jan 22, 2019

Member

*actually need be true

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:user-callbacks branch from 662e715 to 1321c20 Jan 22, 2019

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jan 22, 2019

Updated.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jan 22, 2019

All seems ok, waiting for apveyor just in case

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jan 22, 2019

@GuillaumeGomez Thanks again

@EPashkin EPashkin merged commit a6e229e into gtk-rs:master Jan 22, 2019

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:user-callbacks branch Jan 22, 2019

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.