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

Update for Clang 6 and .NET standard #21

Merged
merged 8 commits into from Jul 7, 2018

Conversation

Projects
None yet
6 participants
@benpye
Contributor

benpye commented Apr 13, 2018

This PR updates for Clang 6 and .NET standard. I have taken some commits from PRs #16 and #18 .

I have moved the projects to .NET standard targets and updated the build.sh build script, though I haven't updated build.bat as of yet. I don't know if it makes sense to keep the mono build method or move entirely to the dotnet CLI.

I have also moved the ClangSharp project so that the simple wildcard .csproj can be used, otherwise the files from ClangSharpPInvokeGenerator would also be taken.

This is a WIP as I believe we should be able to do away with the string wrapper methods introduced in 86aa75f by using a custom marshaller, as far as I can tell using the [return: MarshalAs(UnmanagedType.LPStr)] annotation will result in the string being deallocated, which I believe Clang does not expect as the returns appear to all be const char *.

EDIT: Should now work with edc14c5

I also wonder why two versions of Generated.cs, Generated.Custom.cs and Extensions.cs are kept? Is there any reason we don't use the version in the library for the generator? It would seem better to use the newer versions as it ensures that the library is functional. It could be a useful automated test to ensure that the files present are the same as the files generated.

EDIT: Check b4662c0 for this. Should always keep a working version in the repo for self-hosting but it means that the generated version will be tested by building again which is useful. This still isn't an automated test however.

@msftclas

This comment has been minimized.

msftclas commented Apr 13, 2018

CLA assistant check
All CLA requirements met.

@benpye benpye changed the title from [WIP] Update for Clang 6 and .NET standard to Update for Clang 6 and .NET standard Apr 13, 2018

@amaitland amaitland referenced this pull request Apr 13, 2018

Closed

Upgrade to Clang 5.0.0 #18

@benpye

This comment has been minimized.

Contributor

benpye commented Apr 13, 2018

I have begun to translate some of the upstream libclang tests, whilst these are designed to test the functionality of libclang rather than a binding (for obvious reasons), they still seem like a good source of truth. For reference the upstream tests can be found at https://github.com/llvm-mirror/clang/blob/master/unittests/libclang/LibclangTest.cpp . I have used XUnit but if preferable I see no reason that MSTest couldn't be used instead.

These tests need to be checked on Windows too as I suspect Unicode will be broken there, I believe Clang uses UTF8 everywhere but currently the string encoding is not specified in code. On Linux/MacOS this works as .NET Core defaults to UTF8 there, however on Windows it'll default to ANSI which is likely to break.

It would be nice to get Travis-CI/AppVeyor running the tests too unless you have any preferred testing infrastructure.

@benpye

This comment has been minimized.

Contributor

benpye commented Apr 15, 2018

Looks like switching to ref rather than out was not the right call for libclang, it seems that none of the methods have an [in] parameter, at least if the Doxygen comments are to be trusted. This was previously changed in 204733b from #16 , @blue3k am I missing any reason for the change?

It would be nice though unnecessary to check the comment associated for [in], [out], [in,out] or such so that the decision can be made. In the [out] case we should be using the C# out keyword, otherwise we must map to ref given that there is no in keyword.

I am also not sure why @blue3k removed index_getClientEntity and index_setClientEntity as it seems the current implementation for each should still work (the API doesn't appear to have changed).

benpye and others added some commits Apr 13, 2018

Add --excludeFunctions command line argument
Exclude functions that are manually defined in Generated.Custom.cs
@benpye

This comment has been minimized.

Contributor

benpye commented Apr 15, 2018

Looking at the elaborated type change in #15 and #16 a little further it appears the approach there is incorrect. I think Type_getNamedType is the method that should be called to get the underlying type rather than getCanonicalType as it is not always desirable to get the most basic version of the type. Regardless, the change makes no difference to the Generated.cs file.

@mjsabby

This comment has been minimized.

Member

mjsabby commented Apr 22, 2018

@benpye Thanks for the overhaul work here. Can you explain why the Generated.Custom.cs is no longer needed? I'll take a look at this again tomorrow and merge it in if I don't have any additional questions.

@benpye

This comment has been minimized.

Contributor

benpye commented Apr 22, 2018

I have removed nthe duplicate copy of Generated.cs and Generated.Custom.cs and instead added a dependency on the ClangSharp library. As the latest of each of those files is checked in under that directory with each commit anyway it should be able to bootstrap the generator. The only time an issue might arise is if a broken Generated.cs is written, but in that case it should be possible to use Git to revert to the last good version. This change ensures that the latest version is used with the generator which can acts as a test, before now it looks like the version in the generators code has been allowed to fall behind.

@SolalPirelli

This comment has been minimized.

Contributor

SolalPirelli commented May 8, 2018

@benpye Hi! I'm trying to use this branch... which version of libclang are you using?

Using libclang.dll from the pre-compiled x64 LLVM for Windows, ClangSharpPInvokeGenerator generates getFileContents(CXTranslationUnit @tu, CXFile @file, out size_t @size);, which causes a compilation error because size_t is not defined anywhere. In the committed Generated.cs, the size parameter is an int instead. (Also, the enums are generated with int instead of the current uint as their base type, not sure if this matters)

@benpye

This comment has been minimized.

Contributor

benpye commented May 8, 2018

That's interesting. I have generated on Ubuntu 18.04 with libclang 6.0. I'm a little surprised that symbol is different on Windows, if I get a chance I'll have a look but I probably won't be able to for the next couple weeks as I have exams coming up. It looks like the enum difference is also due to platform differences.

@SolalPirelli

This comment has been minimized.

Contributor

SolalPirelli commented May 8, 2018

I don't have an 18.04 on hand, but libclang-6.0-dev on 16.04 (from http://apt.llvm.org/xenial/) yields the exact same include files as the Windows ones, with CINDEX_LINKAGE const char *clang_getFileContents(CXTranslationUnit tu, CXFile file, size_t *size);. I'll look into it if I have time.

Fixes for clang6 on Windows
* Update build.bat to match build.sh

* Add heuristic for typedefs-in-system-headers detection, fixes size_t issue

* Force newlines to be Unix (to avoid crazy git diffs)

* Update the generated file

* Make VFO test OS-agnostic

* Fix string marshalling since clang uses utf-8; add regression tests.

* Add editorconfig, set newlines/charset everywhere, add .vs to gitignore

* Don't print extra white space at the end of struct fields

* Use dotnet msbuild to build ClangSharp instead of platform-specific scripts

* Force ints for enums if possible, for cross-plat

* Fix interpretation of typedefs, for cross-plat

@benpye benpye referenced this pull request Jul 4, 2018

Closed

Working with DotNetCore #25

@xoofx

This comment has been minimized.

xoofx commented Jul 5, 2018

@mjsabby I would love this PR to land here. Do you know when you will be able to merge it?

@mjsabby

This comment has been minimized.

Member

mjsabby commented Jul 5, 2018

@xoofx I just finishing landing a similar change in LLVMSharp. I intend to finish this before Saturday.

@mjsabby

mjsabby approved these changes Jul 7, 2018

@mjsabby mjsabby merged commit b52de9c into Microsoft:master Jul 7, 2018

1 check passed

license/cla All CLA requirements met.
Details
@mjsabby

This comment has been minimized.

Member

mjsabby commented Jul 7, 2018

The change has landed, thanks @benpye @SolalPirelli @amaitland

A few annoyances remain:

  1. the size_t/int issue that is new in the last few versions.
  2. Passing in the libclang name at Library initialize time, so something like InitializeClangSharp("/my/libclang-X-Y.so")
  3. Automatic nuget generation.
@SolalPirelli

This comment has been minimized.

Contributor

SolalPirelli commented Jul 9, 2018

@mjsabby I fixed 1) in my commit (see here), though it's via a slightly dubious heuristic.

For 3), I guess the main pain point is including all libclang versions in the NuGet package?

@mjsabby

This comment has been minimized.

Member

mjsabby commented Jul 9, 2018

There is also another one that I'm ok with for now since the PR had lots of goodness: The Custom String Marshaller. It adds a dependency on this type which is odd for a code-generator.

@SolalPirelli I saw your fix, I'm hoping we can make it more robust. For (3) yes but we need build support in the repo.

@SolalPirelli

This comment has been minimized.

Contributor

SolalPirelli commented Jul 9, 2018

I have some time to work on improving this library, since I'd like to use it for a project of mine.

@mjsabby re: the custom marshaller:

  • Would it make sense to simply output it as a private nested class within generated classes, to make the output self-contained? (Same remark for Extensions.cs)
  • Are _CXUnsavedFile and _CXIdxEntityInfo still needed in Generated.Custom.cs? They look like they exist specifically to marshal strings in the right way, but now the string marshaller is used for this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment