Skip to content

#590 - Renamed chooser to visitor and added function for parsed macro… #591

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

Merged
merged 1 commit into from
Mar 18, 2017

Conversation

ambaxter
Copy link
Contributor

@ambaxter ambaxter commented Mar 17, 2017

How would you suggest testing this?

@emilio
Copy link
Contributor

emilio commented Mar 17, 2017

You can test it in the bindgen-integration crate. Should be straight-forward. Just create a visitor and add a macro in cpp/Test.h :)

src/lib.rs Outdated
@@ -447,8 +447,8 @@ impl Builder {

/// Allows configuring types in different situations, see the `TypeChooser`
/// documentation.
pub fn type_chooser(mut self, cb: Box<chooser::TypeChooser>) -> Self {
self.options.type_chooser = Some(cb);
pub fn type_chooser(mut self, cb: Box<visitor::BindgenVisitor>) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this not renamed too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@fitzgen
Copy link
Member

fitzgen commented Mar 17, 2017

Apologies for bike shedding, but I'm not in love with the name BindgenVisitor, and although I don't have any great replacements, we do already have visitors internally for dealing with libclang, so "visitor" is now an ambiguous term.

Spitting out anything that comes to mind:

  • TypeVisitor is at least a little more specific
  • OnParse
  • OnType
  • ParseCallbacks
  • ParseGuide
  • TypeBehavior
  • TypeCustomizer
  • ParseBehavior
  • ParseCustomizer

Any other ideas?

@ambaxter
Copy link
Contributor Author

I'm liking onParse or ParseCallbacks, myself.

@emilio
Copy link
Contributor

emilio commented Mar 17, 2017

ParseCallbacks SGTM


#[derive(Debug)]
struct MacroVisitor {
macros: Arc<RwLock<HashSet<String>>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: indentation is off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed


fn main() {
gcc::Config::new()
.cpp(true)
.file("cpp/Test.cc")
.compile("libtest.a");

let macros = Arc::new(RwLock::new(HashSet::new()));

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra newline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@emilio
Copy link
Contributor

emilio commented Mar 17, 2017

This looks fine to me renamed to ParseCallbacks and squashed. Thanks!

Added tests and fixed missed function rename.

Fixed nits

Renamed visitor to callbacks.

Renamed visitor to callbacks.

Renamed visitor to callbacks.

Fixed text.
@ambaxter
Copy link
Contributor Author

Squashed!

@emilio emilio changed the title WIP: #590 - Renamed chooser to visitor and added function for parsed macro… #590 - Renamed chooser to visitor and added function for parsed macro… Mar 18, 2017
@emilio
Copy link
Contributor

emilio commented Mar 18, 2017

@bors-servo r+

Thanks!

@bors-servo
Copy link

📌 Commit 7908561 has been approved by emilio

@bors-servo
Copy link

⌛ Testing commit 7908561 with merge 15a18fa...

bors-servo pushed a commit that referenced this pull request Mar 18, 2017
How would you suggest testing this?
@bors-servo
Copy link

☀️ Test successful - status-travis
Approved by: emilio
Pushing 15a18fa to master...

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.

5 participants