Correctly handle blacklisted items in the template analysis #623
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
|
With the changes in this PR, I can generate bindings for stylo successfully with libclang 3.9. |
Nice! What about libclang 4.0/5.0? Does the regression affect that badly our analysis? |
I haven't tested with other libclang versions. The original test case within bindgen that used default template params looked like it came from gecko though, so if there is any instantiation that relies on the default type, it will fail. Note that this is orthogonal from the template parameter usage analysis. In this scenario, we simply aren't given the data we need, and this happens way before we do any template analysis. |
emilio
reviewed
Apr 7, 2017
| @@ -0,0 +1,13 @@ | |||
| // bindgen-flags: --blacklist-type '^RefPtr$' --whitelist-function 'Servo_.*' --raw-line 'pub type RefPtr<T> = T;' -- -std=c++14 | |||
There was a problem hiding this comment.
There's no need to ^$, it's added implicitly.
The template analysis operates on whitelisted items, and uses our tracing infrastructure to move between them. Usually, that means we can only reach other whitelisted items by tracing, because the set of whitelisted items is the transitive closure of all the items explicitly whitelisted. The exception is when some type is explicitly blacklisted. It could still be reachable via tracing from a whitelisted item, but is not considered whitelisted due to the blacklisting. The easy fix is to run the template analysis on the whole IR graph rather than just the whitelisted set. This is an approximately one line change in the analysis, however is not desirable due to performance concerns. The whole point of whitelisting is that there may be *many* types in a header, but only a *few* the user cares about, or there might be types that aren't explicitly needed and that are too complicated for bindgen to handle generally (often in `<type_traits>`). In these situations, we don't want to waste cycles or even confuse ourselves by considering such types! Instead, we keep the whitelisted item set around and check by hand whether any given item is in it during the template type parameter analysis. Additionally, we make the decision that blacklisted template definitions use all of their type parameters. This seems like a reasonable choice because the type will likely be ported to Rust manually by the bindgen user, and they will be looking at the C++ definition with all of its type parameters. They can always insert `PhantomData`s manually, so it also gives the most flexibility. Fixes rust-lang#584
fitzgen
force-pushed
the
issue-584-stylo-blacklisting-in-template-analysis
branch
from
60ffc31 to
c8a206a
Compare
|
@bors-servo r=emilio Thanks for the review! |
|
📌 Commit c8a206a has been approved by |
bors-servo
pushed a commit
that referenced
this pull request
Apr 10, 2017
…-analysis, r=emilio Correctly handle blacklisted items in the template analysis The template analysis operates on whitelisted items, and uses our tracing infrastructure to move between them. Usually, that means we can only reach other whitelisted items by tracing, because the set of whitelisted items is the transitive closure of all the items explicitly whitelisted. The exception is when some type is explicitly blacklisted. It could still be reachable via tracing from a whitelisted item, but is not considered whitelisted due to the blacklisting. The easy fix is to run the template analysis on the whole IR graph rather than just the whitelisted set. This is an approximately one line change in the analysis, however is not desirable due to performance concerns. The whole point of whitelisting is that there may be *many* types in a header, but only a *few* the user cares about, or there might be types that aren't explicitly needed and that are too complicated for bindgen to handle generally (often in `<type_traits>`). In these situations, we don't want to waste cycles or even confuse ourselves by considering such types! Instead, we keep the whitelisted item set around and check by hand whether any given item is in it during the template type parameter analysis. Additionally, we make the decision that blacklisted template definitions use all of their type parameters. This seems like a reasonable choice because the type will likely be ported to Rust manually by the bindgen user, and they will be looking at the C++ definition with all of its type parameters. They can always insert `PhantomData`s manually, so it also gives the most flexibility. Fixes #584 r? @emilio
|
☀️ Test successful - status-travis |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The template analysis operates on whitelisted items, and uses our tracing
infrastructure to move between them. Usually, that means we can only reach other
whitelisted items by tracing, because the set of whitelisted items is the
transitive closure of all the items explicitly whitelisted. The exception is
when some type is explicitly blacklisted. It could still be reachable via
tracing from a whitelisted item, but is not considered whitelisted due to the
blacklisting.
The easy fix is to run the template analysis on the whole IR graph rather than
just the whitelisted set. This is an approximately one line change in the
analysis, however is not desirable due to performance concerns. The whole point
of whitelisting is that there may be many types in a header, but only a few
the user cares about, or there might be types that aren't explicitly needed and
that are too complicated for bindgen to handle generally (often in
<type_traits>). In these situations, we don't want to waste cycles or evenconfuse ourselves by considering such types!
Instead, we keep the whitelisted item set around and check by hand whether any
given item is in it during the template type parameter analysis.
Additionally, we make the decision that blacklisted template definitions use all
of their type parameters. This seems like a reasonable choice because the type
will likely be ported to Rust manually by the bindgen user, and they will be
looking at the C++ definition with all of its type parameters. They can always
insert
PhantomDatas manually, so it also gives the most flexibility.Fixes #584
r? @emilio