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 TypeKind::Named to TypeKind::TypeParam #915

Closed
wants to merge 1 commit into from
Closed

rename TypeKind::Named to TypeKind::TypeParam #915

wants to merge 1 commit into from

Conversation

anna-liao
Copy link
Contributor

fixes #914. r? @fitzgen

I made the changes and cargo build and cargo test run ok. However, "testing generated bindings" fails. I checked and it seems to be failing even before my changes?

$ cd tests/expectations
$ cargo test

@fitzgen
Copy link
Member

fitzgen commented Aug 15, 2017

However, "testing generated bindings" fails. I checked and it seems to be failing even before my changes?

Huh. Our CI is still passing, and I can run them locally just fine, so I suspect this is a build env issue. Do you have the LIBCLANG_PATH var set while running these tests as well?

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Great start! The changes that exist are perfect, but we missed a few more places :)

See comments below, and also:

  • Item::named_type
  • Type::is_named
  • Type::is_builtin_or_named
  • BindgenContext::add_named_type
  • BindgenContext::get_named_type
  • BindgenContext::named_types

Thanks!

src/ir/ty.rs Outdated
@@ -97,10 +97,10 @@ impl Type {
}
}

/// Is this type of kind `TypeKind::Named`?
/// Is this type of kind `TypeKind::TypeParam`?
pub fn is_named(&self) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

We should rename is_named to is_type_param as well.

src/ir/ty.rs Outdated
@@ -259,7 +259,7 @@ impl Type {
/// tests/headers/381-decltype-alias.hpp
pub fn is_invalid_named_type(&self) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

This should become is_invalid_type_param.

src/ir/ty.rs Outdated
@@ -464,14 +464,14 @@ impl DotAttributes for TypeKind {

#[test]
fn is_invalid_named_type_valid() {
Copy link
Member

Choose a reason for hiding this comment

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

These tests should replace named_type with type_param in their function names.

@anna-liao
Copy link
Contributor Author

thanks for the review! I will update the pull request soon.

Yup, I ran the tests with LIBCLANG_PATH set. I couldn't find libclang.so even searching recursively through that directory, so I didn't set DYLD_LIBRARY_PATH.

tests_expectations output Compiling tests_expectations v0.1.0 (file:///Users/annaliao/repos/rust-bindgen/tests/expectations) error: unions are unstable and possibly buggy (see issue #32836) --> tests/layout_mbuf.rs:90:1 | 90 | / pub union rte_mbuf__bindgen_ty_1 { 91 | | /// < Atomically accessed refcnt 92 | | pub refcnt_atomic: rte_atomic16_t, 93 | | /// < Non-atomically accessed refcnt 94 | | pub refcnt: u16, 95 | | _bindgen_union_align: u16, 96 | | } | |_^

error: unions are unstable and possibly buggy (see issue #32836)
--> tests/layout_mbuf.rs:124:1
|
124 | / pub union rte_mbuf__bindgen_ty_2 {
125 | | /// < L2/L3/L4 and tunnel information.
126 | | pub packet_type: u32,
127 | | pub __bindgen_anon_1: rte_mbuf__bindgen_ty_2__bindgen_ty_1,
128 | | bindgen_union_align: u32,
129 | | }
| |
^

error: unions are unstable and possibly buggy (see issue #32836)
--> tests/layout_mbuf.rs:458:1
|
458 | / pub union rte_mbuf__bindgen_ty_3 {
459 | | /// < RSS hash result if RSS enabled
460 | | pub rss: u32,
461 | | /// < Filter identifier if FDIR enabled
... |
467 | | bindgen_union_align: [u32; 2usize],
468 | | }
| |
^

error: unions are unstable and possibly buggy (see issue #32836)
--> tests/layout_mbuf.rs:477:1
|
477 | / pub union rte_mbuf__bindgen_ty_3__bindgen_ty_1__bindgen_ty_1 {
478 | | pub __bindgen_anon_1: rte_mbuf__bindgen_ty_3__bindgen_ty_1__bindgen_ty_1__bindgen_ty_1,
479 | | pub lo: u32,
480 | | bindgen_union_align: u32,
481 | | }
| |
^

error: unions are unstable and possibly buggy (see issue #32836)
--> tests/layout_mbuf.rs:641:1
|
641 | / pub union rte_mbuf__bindgen_ty_4 {
642 | | /// < Can be used for external metadata
643 | | pub userdata: *mut ::std::os::raw::c_void,
644 | | /// < Allow 8-byte userdata on 32-bit
645 | | pub udata64: u64,
646 | | bindgen_union_align: u64,
647 | | }
| |
^

error: unions are unstable and possibly buggy (see issue #32836)
--> tests/layout_mbuf.rs:675:1
|
675 | / pub union rte_mbuf__bindgen_ty_5 {
676 | | /// < combined for easy fetch
677 | | pub tx_offload: u64,
678 | | pub __bindgen_anon_1: rte_mbuf__bindgen_ty_5__bindgen_ty_1,
679 | | bindgen_union_align: u64,
680 | | }
| |
^

error: aborting due to 6 previous errors

error: Could not compile tests_expectations.
Build failed, waiting for other jobs to finish...
error: build failed

@anna-liao
Copy link
Contributor Author

Thanks for the guidance to fix the remaining "named" occurrences.

I ran "rust fmt" and it flagged a bunch of line length issues, some in files that I didn't modify.

@fitzgen
Copy link
Member

fitzgen commented Aug 16, 2017

I couldn't find libclang.so even searching recursively through that directory

Ok, that's (one of?) the issue(s?). LIBCLANG_PATH needs to point to the directory that contains libclang.so or else bindgen is going to have a bad time.

error: unions are unstable and possibly buggy (see issue #32836)

Have you run rustup update stable in the last few weeks? The latest stable release enabled unions.

@fitzgen
Copy link
Member

fitzgen commented Aug 16, 2017

I ran "rust fmt" and it flagged a bunch of line length issues, some in files that I didn't modify.

That's fine, don't worry about it.

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Thanks! Can you squash these commits into a single commit?

Once that's done, we can land this!

Thanks again!

@bors-servo
Copy link

☔ The latest upstream changes (presumably #916) made this pull request unmergeable. Please resolve the merge conflicts.

@anna-liao
Copy link
Contributor Author

Thanks for the review @fitzgen! I squashed the commits, but I'm not sure if I handled the merge conflicts correctly.

Regarding the libclang.so, I ran rustup update stable and still not finding libclang.so along the directory tree where I'd expect it. I'm trying a full search to see if it is somewhere else.

@bors-servo
Copy link

☔ The latest upstream changes (presumably #922) made this pull request unmergeable. Please resolve the merge conflicts.

fitzgen pushed a commit to fitzgen/rust-bindgen that referenced this pull request Aug 21, 2017
Also renames a bunch of other things referring to named types to refer to type
parameters.

Fixes rust-lang#915
bors-servo pushed a commit that referenced this pull request Aug 21, 2017
Rename `TypeKind::Named` to `TypeKind::TypeParam`

Also renames a bunch of other things referring to named types to refer to type parameters.

Fixes #915
@fitzgen
Copy link
Member

fitzgen commented Aug 21, 2017

Thanks @anna-liao ! I reworded the commit message to be a little more descriptive (you're still marked as author ;) ), and it is landing in #924

Thanks again!

@fitzgen fitzgen closed this Aug 21, 2017
bors-servo pushed a commit that referenced this pull request May 29, 2018
…=emilio

Check if `clang_Type_getNumTemplateArguments` is loaded before use

Fixes #1304, which reapplies #916 which was regressed by #915.
kulp added a commit to kulp/rust-bindgen that referenced this pull request Jun 22, 2020
It is not clear what version of libclang these supported.

Refer to rust-lang#1321, rust-lang#1304, rust-lang#916, rust-lang#915.
kulp added a commit to kulp/rust-bindgen that referenced this pull request Jun 24, 2020
It is not clear what version of libclang these supported.

Refer to rust-lang#1321, rust-lang#1304, rust-lang#916, rust-lang#915.
kulp added a commit to kulp/rust-bindgen that referenced this pull request Jun 28, 2020
It is not clear what version of libclang these supported.

Refer to rust-lang#1321, rust-lang#1304, rust-lang#916, rust-lang#915.
kulp added a commit to kulp/rust-bindgen that referenced this pull request Jun 29, 2020
It is not clear what version of libclang these supported.

Refer to rust-lang#1321, rust-lang#1304, rust-lang#916, rust-lang#915.
kulp added a commit to kulp/rust-bindgen that referenced this pull request Jul 17, 2020
It is not clear what version of libclang these supported.

Refer to rust-lang#1321, rust-lang#1304, rust-lang#916, rust-lang#915.
kulp added a commit to kulp/rust-bindgen that referenced this pull request Jul 20, 2020
It is not clear what version of libclang these supported.

Refer to rust-lang#1321, rust-lang#1304, rust-lang#916, rust-lang#915.
emilio pushed a commit that referenced this pull request Jul 20, 2020
It is not clear what version of libclang these supported.

Refer to #1321, #1304, #916, #915.
LoganBarnett pushed a commit to LoganBarnett/rust-bindgen that referenced this pull request Dec 2, 2023
It is not clear what version of libclang these supported.

Refer to rust-lang#1321, rust-lang#1304, rust-lang#916, rust-lang#915.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename TypeKind::Named to TypeKind::TypeParam
3 participants