Skip to content
This repository has been archived by the owner on Mar 26, 2020. It is now read-only.

Feature/codegen refactor #74

Closed
wants to merge 62 commits into from

Conversation

mknejp
Copy link
Contributor

@mknejp mknejp commented Apr 15, 2015

More details about this can be found here: #70

This includes #40 and #39.

Since this header and all the other support files are located in the same folder this prefix should probably be applied to djinni_support.hpp as well.
This option exists for the JNI support library, so it is only logical to also provide it for the Obj-C part.
The new options --objc-private-out and --objc-include-private-prefix control where the private Objective-C[++] files (i.e. +Private.h and .mm) are placed and which prefix is used to #import or #include them. This allows a clean separation of the public interface and the hidden private parts.

As a bonus this fixes the bug where the Objective-C generator was completely ignoring the --objc-include-prefix and --objc-include-cpp-prefix options.
…nerated

Now it matches the JNI setting. This was probably not supposed to be there...
Nested namespaces were copied verbatim instead of applying namespaces nested as is done in the JNI generator.
This not only reduces the number of public files exposed to users down to one, it also makes things safer. It is no longer possible to pass something to a +c interface from Obj-C that is not compatible and it allows one to directly call static class methods on the generated types instead of going through the CppProxy type.
No longer is the inclusion of any CppProxy or +Private headers required.
Conflicts:
	src/source/ObjcGenerator.scala
Conflicts:
	example/generated-src/objc/TXSSortItems.h
	example/generated-src/objc/TXSSortItems.mm
	test-suite/generated-src/objc/DBClientInterfaceObjcProxy+Private.h
	test-suite/generated-src/objc/DBClientInterfaceObjcProxy.mm
	test-suite/generated-src/objc/DBTestHelpers.mm
	test-suite/objc/DjinniObjcTest.xcodeproj/project.pbxproj
…ng facility

Increases separation of concerns and makes it easier to isolate the parts of code generation that need to be accessed from other language interfaces for interop.
…ns instead of member functions

- Follows the idiomatic way of std::tuple
- Implement operator>, operator<= and operator>= only using operator<
This is only possible because records/interfaces are now generating .mm implementation files even though it is ObjcGenerator creating them. Obj-C++ does not have the "compile time constant" restriction.
This fixes the edge case where one of the constants has the same type as the record defined in the same header.
…factor

# Conflicts:
#	example/generated-src/objc/TXSItemList.h
#	example/generated-src/objc/TXSItemList.mm
#	example/generated-src/objc/TXSSortItemsCppProxy.mm
#	example/objc/TextSort.xcodeproj/project.pbxproj
#	src/source/CppGenerator.scala
#	src/source/JNIGenerator.scala
#	src/source/JavaGenerator.scala
#	src/source/ObjcGenerator.scala
#	src/source/generator.scala
#	support-lib/jni/HString.hpp
#	test-suite/generated-src/objc/DBAssortedIntegers.mm
#	test-suite/generated-src/objc/DBClientReturnedRecord.mm
#	test-suite/generated-src/objc/DBConstants.mm
#	test-suite/generated-src/objc/DBCppExceptionCppProxy.mm
#	test-suite/generated-src/objc/DBMapListRecord.mm
#	test-suite/generated-src/objc/DBMapRecord.mm
#	test-suite/generated-src/objc/DBNestedCollection.mm
#	test-suite/generated-src/objc/DBPrimitiveList.mm
#	test-suite/generated-src/objc/DBRecordWithDerivings.mm
#	test-suite/generated-src/objc/DBRecordWithNestedDerivings.mm
#	test-suite/generated-src/objc/DBSetRecord.mm
#	test-suite/generated-src/objc/DBTestHelpersCppProxy.mm
#	test-suite/generated-src/objc/DBTokenCppProxy.mm
#	test-suite/handwritten-src/objc/tests/DBMapRecordTests.mm
#	test-suite/handwritten-src/objc/tests/DBNestedCollectionTests.mm
#	test-suite/handwritten-src/objc/tests/DBSetRecordTests.mm
#	test-suite/objc/DjinniObjcTest.xcodeproj/project.pbxproj
…factor

# Conflicts:
#	example/generated-src/objc/TXSSortItemsCppProxy+Private.h
#	example/generated-src/objc/TXSSortItemsCppProxy.mm
#	example/generated-src/objc/TXSTextboxListenerObjcProxy+Private.h
#	example/generated-src/objc/TXSTextboxListenerObjcProxy.mm
#	src/source/ObjcGenerator.scala
#	test-suite/generated-src/objc/DBClientInterfaceObjcProxy+Private.h
#	test-suite/generated-src/objc/DBClientInterfaceObjcProxy.mm
#	test-suite/generated-src/objc/DBCppExceptionCppProxy+Private.h
#	test-suite/generated-src/objc/DBCppExceptionCppProxy.mm
#	test-suite/generated-src/objc/DBTestHelpersCppProxy+Private.h
#	test-suite/generated-src/objc/DBTestHelpersCppProxy.mm
#	test-suite/generated-src/objc/DBTokenCppProxy+Private.h
#	test-suite/generated-src/objc/DBTokenCppProxy.mm
@mknejp mknejp mentioned this pull request Apr 15, 2015
@@ -4,8 +4,8 @@ APP_ABI := all
APP_OPTIM := release
APP_PLATFORM := android-8
# GCC 4.8 Toolchain - requires NDK r9
NDK_TOOLCHAIN_VERSION = 4.8
NDK_TOOLCHAIN_VERSION = clang
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason for the lib and compiler changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm actually not sure anymore. I think I was trying it out on both compilers for warnings and probably forgot to discard the change. Maybe it was because of the need for C++14 in the support lib. I don't see a reason why it shouldn't work with GCC

Copy link
Contributor

Choose a reason for hiding this comment

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

Last I tested the 4.8 GCC toolchain was missing std::experimental:optional which prompted the switch in our code.

@j4cbo
Copy link
Contributor

j4cbo commented May 7, 2015

So I'm slowly cherry-picking the individual changes here to go through the necessary updates to our code that uses it. It looks like the JNI codegen change has issues compiling the test suite:

 [exec] In file included from ../generated-src/jni/NativeNestedCollection.cpp:5:
 [exec] ../../support-lib/jni/Marshal.hpp:245:32: error: no member named 'Boxed' in 'djinni::Set<djinni::String>'
 [exec]                 using EJniType = typename T::Boxed::JniType;
 [exec]                                           ~~~^
 [exec] ../generated-src/jni/NativeNestedCollection.cpp:17:70: note: in instantiation of template class 'djinni::List<djinni::Set<djinni::String> >' requested here
 [exec]                                                            ::djinni::List<::djinni::Set<::djinni::String>>::fromCpp(jniEnv, c.set_list).get())};
 [exec]                                                                      ^
 [exec] In file included from ../generated-src/jni/NativeMapListRecord.cpp:5:
 [exec] ../../support-lib/jni/Marshal.hpp:245:32: error: no member named 'Boxed' in 'djinni::Map<djinni::String, djinni::I64>'
 [exec]                 using EJniType = typename T::Boxed::JniType;
 [exec]                                           ~~~^
 [exec] ../generated-src/jni/NativeMapListRecord.cpp:17:70: note: in instantiation of template class 'djinni::List<djinni::Map<djinni::String, djinni::I64> >' requested here
 [exec]                                                            ::djinni::List<::djinni::Map<::djinni::String, ::djinni::I64>>::fromCpp(jniEnv, c.map_list).get())};
 [exec]                                                                      ^

I get the same errors on the latest feature/codegen-refactor in your fork.

Can you rebase the remaining changes on top of current master and fix the compilation issues? Thanks!

@mknejp mknejp mentioned this pull request May 8, 2015
@mknejp
Copy link
Contributor Author

mknejp commented May 8, 2015

Hmm it seems my test setup was borked and the Java tests weren't running properly, weird.
Not every single commit may compile and run tests on its own though, I have to admit that I often do several commits in sequence and then regenerate/run tests before pushing the commits to keep the diffs focused. Not very helpful in this situation.

Anyway, since trying to rebase the existing branch is now giving a bazillion conflicts I'll have to make a new one and try to split it over separate pull requests where I can. Sorry for the inconvenience.

@mknejp mknejp closed this May 8, 2015
@mknejp mknejp deleted the feature/codegen-refactor branch June 24, 2015 12:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants