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

Extended the support for ClangSharp via libClangSharp #159

Merged
merged 12 commits into from
Jul 6, 2020

Conversation

tannergooding
Copy link
Member

The libClang bindings are very limited in what they expose and libClangSharp exists to help bridge that gap.

This expands libClangSharp to support quite a bit of additional functionality so the much of the guesswork no longer has to occur. Namely, the majority of the core properties on the C and C++ decls, statements, and expressions now have a way to directly query the relevant information. ObjC, OMP, and various Clang specific builtins do not currently have such support nor is absolutely everything exposed. However, this should make traversal of more complex ASTs easier and less error prone. It should also help with processing information out of the various template types.

Copy link
Contributor

@PathogenDavid PathogenDavid left a comment

Choose a reason for hiding this comment

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

Lots of great stuff being added!

Found some issues as I was reading through things, but I was by no means thorough. (Although I was definitely paying more attention from the first comment onwards.)

sources/libClangSharp/CXSourceLocation.cpp Outdated Show resolved Hide resolved
sources/libClangSharp/ClangSharp.cpp Show resolved Hide resolved
sources/libClangSharp/ClangSharp.cpp Outdated Show resolved Hide resolved
sources/libClangSharp/ClangSharp.cpp Outdated Show resolved Hide resolved
sources/libClangSharp/ClangSharp.cpp Outdated Show resolved Hide resolved
sources/libClangSharp/ClangSharp.cpp Show resolved Hide resolved
sources/libClangSharp/ClangSharp.cpp Show resolved Hide resolved
sources/libClangSharp/ClangSharp.cpp Show resolved Hide resolved
sources/libClangSharp/ClangSharp.cpp Show resolved Hide resolved
@@ -12,7 +14,7 @@
#endif

#ifdef _MSC_VER
#ifdef CLANGSHARP_LINKAGE
#ifdef clangsharp_LINKAGE
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be libClangSharp_EXPORTS?

(libClangSharp because that's what it's called on Windows builds and this is for Windows.

Better yet, can we just add our own preprocessor define instead of relying on the magic one added by CMake?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, libclang just uses CLANG_LINKAGE

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm slightly confused now. Pretty sure libclang uses CINDEX_LINKAGE and that's not what this review comment is referring to.

I see you changed it back to #ifdef CLANGSHARP_LINKAGE, but I don't think that's right either.

#ifdef CLANGSHARP_LINKAGE
#define CLANGSHARP_LINKAGE EXTERN_C __declspec(dllexport)
#else
#define CLANGSHARP_LINKAGE EXTERN_C __declspec(dllimport)
#endif

This can only ever do two things:

  1. CLANGSHARP_LINKAGE will be defined twice, causing an C4005 warning.
  2. CLANGSHARP_LINKAGE will defined as EXTERN_C __declspec(dllimport) (which isn't right when we're building a DLL.)

I feel like I'm missing something here since it shouldn't even build without CLANGSHARP_LINKAGE (and even with it defined there'll be a warning.) Maybe I'm blind, but it doesn't look like libClangSharp is even built by the build scripts at all. Are you just manually invoking cmake and creating the NuGet package for it manually?

Copy link
Member Author

@tannergooding tannergooding Jul 6, 2020

Choose a reason for hiding this comment

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

It looks to be "working" because CLANGSHARP_LINKAGE isn't defined and therefore it is always exported.
Edit: Got this reversed, but in any case, MSVC is still correctly exporting the symbols and things work, so it isn't really an issue.

It ultimately doesn't matter because no one is going to use this from C code anyways, so most of it doesn't apply.
I will, however, fix it to just always export with a note that no one should be using this from another C library.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good

@tannergooding tannergooding merged commit d82f320 into dotnet:master Jul 6, 2020
@tannergooding tannergooding deleted the extend-func branch July 6, 2020 20:15
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.

2 participants