LLVMSharp icon indicating copy to clipboard operation
LLVMSharp copied to clipboard

Proposing moving the OOAPI to a new namespace

Open TChatzigiannakis opened this issue 8 years ago • 6 comments

The LLVMSharp namespace is starting to get too crowded, which is made worse by the fact that there are 2 types for each thing (LLVMValueRef, Value, etc). Especially the class LLVM might get awkward if we start including some static methods that work with the equivalent class types and return the equivalent class types.

  • I propose moving the class-based API to a new namespace (e.g. LLVMSharp.API) along with its own, cleaner version of the LLVM class (that contains only references to the wrapper classes).
  • We can keep the struct-based API where it is (or we can move it to something like LLVMSharp.Bindings, for symmetry -- if this breaking change isn't too much).

TChatzigiannakis avatar Apr 10 '16 08:04 TChatzigiannakis

LLVMSharp.Api? Capitals is reserved in my opinion for nouns.

Let's not change the struct-based APIs, it's a breaking change and there are lots of users of this API.

Pretty soon I'd want us to move the OO APIs to master. I'm using the OO APIs extensively now and they are much better to work with.

If you are planning to do this, what timeframe are you thinking to get this landed?

mjsabby avatar Apr 10 '16 09:04 mjsabby

  • I suggested API because it looks more like the convention we already have in the name LLVMSharp. But Api is what the MSDN Capitalization Conventions seem to suggest (in accordance with .NET identifiers like TcpClient, HttpClient, etc). I'm okay with either option, or with any other identifier, so let's go for Api.
  • You're right in that it's better to leave the sturct-based API where it is, to maintain a stable public interface there, so I'll leave those untouched.

If you agree, I can probably push this change within the next few days.

TChatzigiannakis avatar Apr 10 '16 11:04 TChatzigiannakis

Cool. Sounds good to me.

mjsabby avatar Apr 10 '16 14:04 mjsabby

I've started doing this and as I mentioned, in order to make the LLVMSharp.Api namespace standalone (that is, not require the user to include LLVMSharp as well), I must "duplicate" the remaining functionality of the LLVM class (by exposing whichever functions aren't in a class already, e.g. EnablePrettyStackTrace).

As I said before, I could do this by introducing an LLVMSharp.Api.LLVM static class again, but I'm thinking maybe I could instead introduce one static class per header file in which the LLVM-C functions are declared. For example:

  • EnablePrettyStackTrace will be accessible through the LLVM class (when importing the LLVMSharp namespace) and through a static class named ErrorHandling (when importing the LLVMSharp.Api namespace).
  • LoadLibraryPermanently will be accessible through the LLVM class (when importing the LLVMSharp namespace) and through a static class named Support (when importing the LLVMSharp.Api namespace).

And so on.

Apart from avoiding to define a new class to contain our "remaining" features (which will probably be unrelated with each other), it will also allow the user to find documentation a bit more easily.

If you agree, I'll perform this change along with the moving of the existing classes, in order to have a standalone LLVMSharp.Api namespace (hopefully) in the next commit.

TChatzigiannakis avatar Apr 10 '16 14:04 TChatzigiannakis

I mostly am fine with it, just curious how many such functions are there? I feel like the user having to include two namespaces isn't the end of the world. But maybe I'll be convinced by your code change.

So yeah go ahead, but I'd like to see the end result before fully committing ..

mjsabby avatar Apr 10 '16 22:04 mjsabby

It's certainly easy to include two namespaces in general. But if the user has to do that, we're back to the position where they'll see duplicate types all around. So if they choose to work with the class-based API, they'd import a huge amount of methods and half of them won't be applicable. This is what I wanted to solve with the namespace separation.

In any case, see the PR I'm making now.

TChatzigiannakis avatar Apr 11 '16 08:04 TChatzigiannakis