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

Clean up the API and document everything #23

Open
sethfowler opened this issue May 13, 2013 · 9 comments
Open

Clean up the API and document everything #23

sethfowler opened this issue May 13, 2013 · 9 comments

Comments

@sethfowler
Copy link
Collaborator

This library should remain a thin layer over the C libclang API, but there are definitely some cases where things can be made a bit easier to use, and everything needs to be documented. Examples:

  • Some thought needs to be given to what should be exported by default in the top-level Clang module.
  • It'd be good to go over the names of all the functions and make sure they're consistent. There are some functions that might be better off in a different module.
  • If there are places where we can reduce the need to manually write out the ClangApp phantom type parameter, that'd be nice.
  • Everything needs to be documented.
@sethfowler
Copy link
Collaborator Author

@ghorn, since you've been using the library recently, I'd love any feedback you have to offer about things that could be improved to make the library easier to use. Try out the llvm3.3 branch so you can see the state of current development.

The ClangApp monad has changed a bit. Note that your functions that previously had the type a -> ClangApp b now need the type a -> ClangApp s b. For example one function in my application has the type Cursor -> ClangApp s Identifier. The s is a phantom type parameter like that used for the ST monad.

@ghorn
Copy link
Contributor

ghorn commented May 13, 2013

Hi Seth. I've been using safer-api for a few days, and I've found it a dramatic improvement. I don't see llvm3.3 on your account, only on chetant's account. Should I checkout that one? I also see some other branches on your account which you might want to delete if they're fully merged.

@sethfowler
Copy link
Collaborator Author

The llvm3.3 branch here (chetant's account) is based on the safer-api branch on my account. I'd recommend switching to the branch here.

@ghorn
Copy link
Contributor

ghorn commented May 15, 2013

I am supposed to build llvm3.3 from their svn trunk, right? There is no release or tagged svn version for 3.3?

@sethfowler
Copy link
Collaborator Author

I think there is a tagged version for 3.3, though don't quote me on that, but building it against trunk would be fine. (I actually use their git mirror.)

If it's an inconvenience don't worry about it. I hope to get libclang embedded sometime next week so these sorts of considerations become a thing of the past.

@sethfowler
Copy link
Collaborator Author

@chetant So in my review this is the primary release blocker, now that (hopefully) GHCi is working on both OS X and linux. (We also need to resolve whatever build issues you mentioned you were having on Ubuntu 13.04.)

My plans here are basically these:

  • Separate the types declared by the FFI module into a different module, Clang.Internal.Types.
  • Export those types, along with the current content of Clang.Monad and Clang.Traversal, from the root Clang module. Don't export them from the more specific modules like Clang.Cursor. This means that we can reuse names like getSpelling and getType in different modules (since modules like Clang.Cursor will be imported qualified) but users don't have to qualify the types as well, which doesn't offer much benefit.
  • Add a few utility functions where they might improve the user experience. I've written a parseSourceFile function that should be much easier to figure out for beginners than the combination of withIndex and withParse that you need now. That was an obvious gap to me. I haven't thought of any others at the moment, but maybe more will jump out.
  • Make sure that all functions are placed in the most logical module.
  • Add Haddock docs for Clang.hs.
  • Add Haddock docs for Comment.hs.
  • Add Haddock docs for Completion.hs.
  • Add Haddock docs for Cursor.hs.
  • Add Haddock docs for Debug.hs.
  • Add Haddock docs for Diagnostic.hs.
  • Add Haddock docs for File.hs.
  • Add Haddock docs for Module.hs.
  • Add Haddock docs for Remapping.hs.
  • Add Haddock docs for Source.hs.
  • Add Haddock docs for String.hs.
  • Add Haddock docs for Token.hs.
  • Add Haddock docs for TranslationUnit.hs.
  • Add Haddock docs for Type.hs.
  • Add Haddock docs for USR.hs.
  • Add Haddock docs for Version.hs.

I think that should cover all my concerns, and from my perspective we'll be ready to make a release on Hackage once this is done.

Does this sound good to you? Are there any other problems we need to tackle?

@chetant
Copy link
Owner

chetant commented Apr 15, 2014

That makes sense. Will tackle the build issues on Linux, then work with you
to cleanup and document stuff.
Thanks for updating the api and cleaning it up considerably Seth!
On Apr 15, 2014 5:16 PM, "Seth Fowler" notifications@github.com wrote:

@chetant https://github.com/chetant So in my review this is the primary
release blocker, now that (hopefully) GHCi is working on both OS X and
linux. (We also need to resolve whatever build issues you mentioned you
were having on Ubuntu 13.04.)

My plans here are basically these:

[ ] Separate the types declared by the FFI module into a different
module, Clang.Internal.Types.
[ ] Export those types, along with the current content of Clang.Monad and
Clang.Traversal, from the root Clang module. Don't export them from the
more specific modules like Clang.Cursor. This means that we can reuse names
like getSpelling and getType in different modules (since modules like
Clang.Cursor will be imported qualified) but users don't have to qualify
the types as well, which doesn't offer much benefit.
[ ] Add a few utility functions where they might improve the user
experience. I've written a parseSourceFile function that should be much
easier to figure out for beginners than the combination of withIndex and
withParse that you need now. That was an obvious gap to me. I haven't
thought of any others at the moment, but maybe more will jump out.
[ ] Make sure that all functions are placed in the most logical module.
[ ] Add Haddock docs for every type and function. Most of them can be
brought over from the underlying libclang's Index.h, but they'll
naturally require some cleanup.

I think that should cover all my concerns, and from my perspective we'll
be ready to make a release on Hackage once this is done.

Does this sound good to you? Are there any other problems we need to
tackle?


Reply to this email directly or view it on GitHubhttps://github.com//issues/23#issuecomment-40541357
.

@sethfowler
Copy link
Collaborator Author

I'm glad I could help!

sethfowler added a commit that referenced this issue Apr 16, 2014
@sethfowler
Copy link
Collaborator Author

I checked off the first point about Clang.Internal.Types, but though I experimented with that approach, I decided against it. It turned out to be much more painful than I expected - Greencard isn't really set up to make this sort of split nice. It may be worth revisiting in the future, but for now I just changed where the types were exported without actually moving them to a new module.

Now that I've done that, though, I'm not sure it was the best move for every type. Many of the enumerations, in particular, seem like they might make more sense in a specific module after all. I do think this continues to look like the right choice for core types (TranslationUnit, Cursor, Type, etc.), though.

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

No branches or pull requests

3 participants