Skip to content

Feedback on using a generic iterator for clang.rs (Not for merge) #189

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

Closed
wants to merge 5 commits into from

Conversation

jeanphilippeD
Copy link
Contributor

Hi,
In the issues I fixed before I've added 3 iterators in clang.rs which are mostly duplicate code.
It seems there is room to have many of such iterators in this file, and maybe we want to avoid the overhead of having these duplicated, so we can use them in all the places where it makes sense.
They also had the issue that the clang_*_getNum and clang_*_get* calls where far apart.

The first commit added generic code and tests.
The last 3 commits replace our existing iterator implementation.

This is just a proof of concept, and If this is something we want, I'll think I could use some guidance to make it in a changeset we want to merge.

Kind Regards,
Jean-Philippe.

Some questions that came up:

  • Is it something we want?
  • Is there a better way to do it?
  • Am I actually achieving the goal of compile time dispatch that can be optimised away?
  • Where should the generic code go? (same file, other file?)
  • The test structs/impl and test function where helpful for me to develop it, but it seems no other tests are run this way: Do we want to keep these? where should it go?
  • I did not mean for anything to be able to access IndexCallIterator (would break assumption if something change the length value) is it ok as it is?

Context:
Coming from C++ I thought template, and I gave my best attempt at this idea getting inspiration from the iterator trait.

My goals where:

  • Compile time dispatch (no virtual function call)
  • The unsafe call to clang_*_getNum and clang_*_get* should be close together for readability
  • These calls should not cluttered by details
  • The impl function to get iterator should not have much noise either.
  • Support both type of get iterator: Option(for signed getNum with -1 value) and non Option(for unsigned getNum)
  • Making a new iterator should be simple and the compiler should protect you from error (signed vs unsigned getNum)
  • Generic code is reasonably simple.

@highfive
Copy link

highfive commented Nov 1, 2016

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

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

This looks pretty neat! I'd probably simplify the trait removing ItemNum and returning Option<usize> directly.

Apart from that, I'm not sure IndexCallable is a great name, but I can't think of any better either so... :)

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.

This is heading in a good direction!

Holler when you think this is ready to land and @emilio or I can do a proper review of the PR.

}
}

struct TestIndexCallableProvider {
Copy link
Member

Choose a reason for hiding this comment

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

All the testing stuff should be contained in a sub-module at the bottom of this file, like this:

#[cfg(test)]
mod tests {
    // testing stuff here...
}

@@ -1170,6 +1170,157 @@ impl UnsavedFile {
}
}

/// Provide index results.
pub trait IndexCallable {
Copy link
Member

Choose a reason for hiding this comment

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

FfiIndexable? FfiIterable?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants