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

Clang 12 + linux support, project improvements #3

Merged
merged 19 commits into from
Oct 26, 2020
Merged

Conversation

Hexlord
Copy link
Contributor

@Hexlord Hexlord commented Oct 18, 2020

Updated cmake to work with ninja on linux (tested under ubuntu 20.04)
Updated code to:

  • work with latest clang libtooling, be able to build via clang on linux
  • correctly handle destructors
  • generate reflection for methods and their params
  • reflect and serialize static arrays and std::array including arrays of arrays
  • fixes some bugs
  • ran autoformat (sorry about that)
  • added .gitignore lines for CLion workflow

Improtant:
Due to changes to cmake source generation, first fresh build results in failure due to source files being overwritten during project compilation: ProduceMetaCPP and Example are built in parallel, and I know of no way to prevent that from cmake (I really searched), but maybe some post-cmake-generation makefile injection could do the trick - but this is a dangerous territory. In my build system I fail the build from my old analogue of MetaCPP, saying that "Hey, I just changed the .generated source, you might want to restart the build, so that your actual project recompiles it".
Without the changes, though, ninja is not supported. I would perfectly understand you not wanting to use these changes in case you are comfortable with MSVC sole setup.

Also I am not sure about the flag I am adding with the include path, but I do not think it would hurt. It is the path clang resides in when built from sources and installed on linux.

…etic fixes; Template specialization in template argument as qualtype(important fix); a bit more tests (18.10 00:44)
…on; some fixes; using tabs; autoformated everything (18.10 14:09)
@Hexlord
Copy link
Contributor Author

Hexlord commented Oct 18, 2020

Also this(Hexlord@b245edb) commit fixes infinite recursion for my project usage (it was something like struct Some : public struct Templ), I think I introduced this in the first place when allowed decl-only for templ specializations, fyi

UPD: wow markdown hid my example:
struct Some : public struct Templ< Some >

@Hexlord
Copy link
Contributor Author

Hexlord commented Oct 21, 2020

Okay I think I finally grokked your codestyle settings, so my reformat does not tear code base apart, just forcepushed my best effort
Also included the infinite recursion fix.

@mlomb
Copy link
Owner

mlomb commented Oct 22, 2020

The setup you propose with ProduceMetaCPP is a good solution. I think it is even better with MSVC, since I can rebuild the Example project without triggering a full meta generation which can take a few seconds (metadata could be stale, so it should be used with caution).

I configured a basic CI workflow to execute the Example program. I'm testing the workflow in this repo, I'll delete it after the PR is merged. As you can see in this build it seems that ProduceCPP is failing to generate the metadata but the process is still exiting normally (it should exit with 1). This is a problem with MetaCPPCLI itself, because ScraperTool.Run doesn't return errors. The error produced is this:

[ 85%] Generating MetaCPP
In file included from /home/runner/work/MetaCPP-workflow-test/MetaCPP-workflow-test/Example/objects.hpp:1:
In file included from /usr/lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/string:40:
In file included from /usr/lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/char_traits.h:40:
In file included from /usr/lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/postypes.h:40:
In file included from /usr/lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/cwchar:44:
/usr/include/wchar.h:35:10: fatal error: 'stddef.h' file not found
#include <stddef.h>
         ^~~~~~~~~~
[Unsupported TypeClass: FunctionProto]
1 error generated.
Error while processing /home/runner/work/MetaCPP-workflow-test/MetaCPP-workflow-test/Example/objects.hpp.
[ 85%] Built target ProduceMetaCPP

Is it normal that it uses the includes from GCC? I didn't really tested it on Linux at all.
We don't have to fix the process exit code now, but we should make sure the Example program generates the correct metadata in the workflow.

Give me a few more days to review the changes and we're set :).

Edit: It seems workflows created in PRs don't trigger until the workflow file is merged 😞 .
Edit 2: nevermind, it seems it runs in your repo. And it fails for some reason, oh well. I'll check it another day.

@Hexlord
Copy link
Contributor Author

Hexlord commented Oct 22, 2020

Hey!

Yes, I was also unable to use MetaCPP at linux until I added following flags:
meta_generate(A1 "${A1_SOURCE}/Application/Application.h" "${A1_SOURCE}/Generated/Generated.hpp" "${A1_SOURCE}/Generated/Generated.cpp" "--flag -I/usr/local/lib/clang/12.0.0/include --flag -DPOSIX --flag -DLINUX ${A1_ALL_INCLUDE_METACPP}")

Important part being "-I/usr/local/lib/clang/12.0.0/include" - this is the path where my clang is installed locally
I probably should have kept this (34172ad) with the line flags.push_back("-I/usr/local/lib/clang/12.0.0/include"); change which I removed and added to my own CMakeLists, but not the example's, my bad really (but I am not sure your path would be the same really)
I will try making your repo work later today

Edit: Yes, it is normal for clang to use GCC includes. It could be configured to use clang own's stl implementation, but for some reason it is not the default behaviour, but I am no expert at this.

Edit2: stddef.h - I had this error, if I recall correctly, I fixed it with "-xc++", which is still present if code flags preparation, I wonder if this change (f7e551b) broke it (# add_definitions(${LLVM_COMPILE_FLAGS})) - the flags are commented out, but I am not really sure what is happening just yet, will look soon enough

@mlomb
Copy link
Owner

mlomb commented Oct 22, 2020

I commented the flags because if LLVM is compiled with std=c++14 it will be present in LLVM_COMPILE_FLAGS and override the std=c++17 and fail to compile. Then the flag -fno-rtti didn't match, so I explicitly set it.

@mlomb
Copy link
Owner

mlomb commented Oct 22, 2020

I added the exit code for MetaCPPCLI, now it fails on the stddef.h error as expected (build).

@mlomb
Copy link
Owner

mlomb commented Oct 22, 2020

We don't have to fix it today, take it easy =)

@Hexlord
Copy link
Contributor Author

Hexlord commented Oct 22, 2020

It is fine, it kinda works now (build on my pc and in the container), deserialization is actually broken for static arrays it seems, guess will look at it tomorrow
Also probably gotta squash these commits of mine :)

@Hexlord
Copy link
Contributor Author

Hexlord commented Oct 26, 2020

So I reimplemented the thing with the recursion, and added ARRAY qualifier, and it worked (reserialization matched) for array and array of arrays. But array of arrays of pointers did not work, because the element type was interpreted as char[2] (instead of char* [2]), because qualifiers are dropped in reflection types (so that we reflect the types, not the pointers to types and other stuff like that, I totally dig your approach, but it seems it brings serious limitations).

To make this work we have to support both the instantiation type qualifiers, and the type qualifiers, so that we could support char (arrayOfPointersToArrays[2])[2] , and just char [2][2].

To support it we need to add vector of qualifiers to Type, namely - variant of (pointer, ref, array(n)).

I will try to implement it, but it is a serious architecture change, and would probably take some time for me to do.
Maybe we need to forget about arrays of pointers and more complex stuff, and I should just push the version from above with the ARRAY qualifier, but it is not the extensible solution.

For my needs, I don't even need pointers, I use handles everywhere in GPP, but I do use arrays and bitfields, so it is a top priority for me. What do you think? @mlomb

@mlomb
Copy link
Owner

mlomb commented Oct 26, 2020

Implementing that solution would requires some time and I don't have many right now, if this PR fills your needs, we can merge it and leave the array of arrays of pointers problem for another time.
Updates:

  • Example: changed magic_numbers back to std::vector<int>.
  • README: to refer to this issue is still pending and some cleaning
  • Minor style and formatting fixes

I'm ready to merge, please confim

@Hexlord
Copy link
Contributor Author

Hexlord commented Oct 26, 2020

Okay I pushed the version where it works for char* C, char C[2], and char C[2][2], but not char C[2][2][2] and not C*[2][2].
Removed my asserts, I think it is good to go. It supports more than without this PR, so I hope it is a good change.

@mlomb mlomb merged commit f370225 into mlomb:master Oct 26, 2020
@mlomb
Copy link
Owner

mlomb commented Oct 26, 2020

And It's merged. Thanks for your contribution and time spent! 😄

Closes #2

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