Store types in ir #68

kornilova203 · 2018-06-24T20:00:21Z

Closes #52

Added multiple new classes in ir/types directory. I decided not to wrap struct/union/enum into a Type class instance but inherit the classes from Type.

One of changes:
Typedefs for struct/union/enum are generated right after a declaration was visited.
Example of such typedef:

type struct_point = native.CStruct2[native.CInt, native.CInt]

Created typedef points to struct/union/enum instance and it is added to translation map, so all other declarations that use type of this struct/union/enum will point to the created typedef.

I will carefully review the code later.

jonas · 2018-06-25T04:35:23Z

tests/samples/Struct.scala

  type struct_point = native.CStruct2[native.CInt, native.CInt]
+  type point_s = native.Ptr[struct_point]


jonas · 2018-06-25T04:35:26Z

tests/samples/Typedef.scala

  type toggle_e = enum_toggle_e
  type int2int = native.CFunctionPtr1[native.CInt, native.CInt]
  type day2string = native.CFunctionPtr1[enum_days, native.CString]
  type toggle = native.CFunctionPtr1[toggle_e, Unit]
-  type enum_days = native.CUnsignedInt
-  type enum_toggle_e = native.CUnsignedInt


It looks like you are doing a lot of refactoring. Would you mind to split out things like reordering of the generated code to a separate PR?

Generated code is reordered because typedefs are now generated earlier. Order of types happens to be more logical but it is not a separate refactoring

jonas · 2018-06-25T04:36:11Z

bindgen/ir/types/SimpleType.h

+/**
+ * For example native.CInt
+ */
+class SimpleType : public Type {


I suggest to name it PrimitiveType.

Name is good

kornilova203 · 2018-06-25T12:33:22Z

I tried to do memory management properly but got a problem:
Type instances are stored in other types and in such objects as Field. They can be deleted if they are not contained in IR (struct, union, enum or typedef). But the problem is that vector<Field> may copy-construct Fields and delete them that triggers destructor of Type.

It is not possible to use Type values instead of pointers because inheritance will not work properly. Using vectors of pointers for everything (so destructors are not called by vector) also does not seem to be a good idea.

I decided to not spend too much time on it and leave it as it is.

jonas · 2018-06-25T16:42:31Z

Okay, maybe we should create a ticket to investigate sometime in the future. I will take a second look when #62 has been merged and the conflict in this PR has been fixed.

jonas

Make a second pass. Impressive work.

jonas · 2018-06-27T20:49:27Z

bindgen/CycleDetection.h

-        std::string qtpeString = tpeTransl.Translate(qtpe);
+        Type *type = tpeTransl.translate(qtpe);
+        if (type == nullptr) {
+            /* temp fix for function pointer */


What temporary fix?

The function recursively extracts pointee type from pointer type to add a dependency.
So it extracts FunctionType from FunctionPointerType but TypeTranslator is unable to translate FunctionType
I'll make the comment more clear

jonas · 2018-06-27T20:51:44Z

bindgen/TypeTranslator.cpp

@@ -27,38 +30,24 @@ TypeTranslator::TypeTranslator(clang::ASTContext *ctx_) : ctx(ctx_), typeMap() {
    typeMap["char32_t"] = "native.CChar32";
    typeMap["float"] = "native.CFloat";
    typeMap["double"] = "native.CDouble";
-    typeMap["void*"] = "native.Ptr[Byte]";


jonas · 2018-06-27T20:53:22Z

bindgen/TypeTranslator.cpp

-        return std::string("native.CFunctionPtr") + std::to_string(counter) +
-               "[" + params + variad + ret + "]";
+        return new FunctionPointerType(returnType, parametersTypes,
+                                       fc->isVariadic());


jonas · 2018-06-27T20:59:38Z

bindgen/visitor/TreeVisitor.cpp

-    ir.addEnum(name, typeTranslator.Translate(enumdecl->getIntegerType()),
-               enumerators);
+    std::string scalaType = typeTranslator.getTypeFromTypeMap(
+        enumdecl->getIntegerType().getUnqualifiedType().getAsString());


Would be nice to get rid of this string translation.

jonas · 2018-06-27T21:00:46Z

bindgen/TypeTranslator.cpp

        }

        // Take care of char*
        if (as->getKind() == clang::BuiltinType::Char_S ||
            as->getKind() == clang::BuiltinType::SChar) {
-            return "native.CString";
+            return new PrimitiveType("native.CString");


Could this be a constant? Similar for the other "native.$type".

Also CString is technically char * so more logically would be encoded as new PointerType(new PrimitiveType("native.CChar")). Would you mind leaving a FIXME or comment related to this?

jonas · 2018-06-28T02:24:51Z

bindgen/TypeTranslator.cpp

-std::string
-TypeTranslator::TranslateConstantArray(const clang::ConstantArrayType *ar,
-                                       const std::string *avoid) {
+Type *TypeTranslator::translateEnum(const clang::QualType &qtpe) {


It looks like this method is reused actually be used by translateStruct or even merged with it once the union specific if branch is removed.

jonas · 2018-06-28T02:25:49Z

bindgen/TypeTranslator.cpp

        } else {
-            // TODO: Properly handle non-default types
-            return handleReservedWords(qtpe.getUnqualifiedType().getAsString());
+            return ir.getTypeDefWithName(


I guess here we don't want to go through the alias map?

No, because struct myStruct will be translated above (the same for enum and union)

jonas · 2018-06-28T02:32:28Z

bindgen/TypeTranslator.h

+    /**
+     * Maps C struct, union or enum name to Type alias
+     */
+    std::map<std::string, Type *> aliasesMap;


I quite liked the unification in the original typeMap. Would it make sense to change it to be defined as:

std::map<std::string, Type *> typeMap

populated with

typeMap["void"] = new PrimitiveType("Unit"); typeMap["bool"] = new PrimitiveType("native.CBool"); typeMap["_Bool"] = new PrimitiveType("native.CBool"); // ...

I assume the problem is with the deallocation?

Yes, deallocation is the problem (well, these instances are not deallocated now, but changing type of the map will make fixing it harder)

I'll create an issue for deallocation

Or simply do it now, because I do not want the code that does not deallocate objects

jonas · 2018-06-28T02:46:38Z

bindgen/defines/DefineFinder.cpp

@@ -109,60 +111,63 @@ void DefineFinder::addNumericConstantDefine(const std::string &macroName,
                                            const clang::Token &token,
                                            bool positive) {
    clang::NumericLiteralParser parser(literal, token.getLocation(), pp);
-    std::string type;
+    Type *type = nullptr;


This method could be simpler by using std::string typeName and still use "native.CLongLong etc. and do the new PrimitiveType(typeName) allocation at the end.

jonas · 2018-06-28T02:59:48Z

bindgen/ir/types/Type.cpp

+#include "Type.h"
+#include "../../Utils.h"
+
+std::string Type::str() const { return handleReservedWords(_str()); }


Won't this call handleReservedWords() on stuff like native.CFunctionPtr[...]? It seems that it over-do it. How hard would it be to selectively call it for any type that returns a simple name and then get rid of _str()?

It shouldn't be a problem

kornilova203 · 2018-06-28T09:27:03Z

Now all objects are deallocated. I checked it with Valgrind

kornilova203 · 2018-06-28T09:30:36Z

But it is a bit ugly because some types are shared between objects (Struct, Union, Enum and TypeDef) and other Types are stored in one object.
Now I think that it would be better not to inherit Struct (an other IR types) from Type but instead create wrappers of type Type so Type instances will not be created for each object and will not be shared. I hope it will make code less complicated

jonas

Stellar work!

jonas reviewed Jun 25, 2018

View reviewed changes

kornilova203 force-pushed the store-types-in-ir branch from 3217dfd to 51f57bf Compare June 25, 2018 12:25

kornilova203 changed the title ~~[WIP] Store types in ir~~ Store types in ir Jun 25, 2018

kornilova203 added 3 commits June 27, 2018 09:58

Do not save type translations

40a4d0e

Store types in intermediate representation

494e4d9

Rename SimpleType and remove unused imports

8db4611

kornilova203 force-pushed the store-types-in-ir branch from 51f57bf to 8db4611 Compare June 27, 2018 06:25

kornilova203 requested a review from jonas June 27, 2018 06:44

jonas reviewed Jun 28, 2018

View reviewed changes

kornilova203 added 2 commits June 28, 2018 11:45

handleReservedWords only where needed and small fixes

4d5fe4b

Deallocate objects

5ae0664

jonas approved these changes Jun 28, 2018

View reviewed changes

kornilova203 merged commit 4d30963 into master Jun 28, 2018

kornilova203 deleted the store-types-in-ir branch June 28, 2018 14:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store types in ir #68

Store types in ir #68

kornilova203 commented Jun 24, 2018

jonas Jun 25, 2018

jonas Jun 25, 2018

kornilova203 Jun 25, 2018

jonas Jun 25, 2018

kornilova203 Jun 25, 2018

kornilova203 commented Jun 25, 2018

jonas commented Jun 25, 2018

jonas left a comment

jonas Jun 27, 2018

kornilova203 Jun 28, 2018

jonas Jun 27, 2018

jonas Jun 27, 2018

jonas Jun 27, 2018

jonas Jun 27, 2018

kornilova203 Jun 28, 2018

jonas Jun 28, 2018

kornilova203 Jun 28, 2018

jonas Jun 28, 2018

kornilova203 Jun 28, 2018

jonas Jun 28, 2018

kornilova203 Jun 28, 2018

kornilova203 Jun 28, 2018

kornilova203 Jun 28, 2018

jonas Jun 28, 2018

kornilova203 Jun 28, 2018

jonas Jun 28, 2018

kornilova203 Jun 28, 2018

kornilova203 commented Jun 28, 2018

kornilova203 commented Jun 28, 2018 •

edited

Loading

jonas left a comment

		type struct_point = native.CStruct2[native.CInt, native.CInt]
		type point_s = native.Ptr[struct_point]

Store types in ir #68

Store types in ir #68

Conversation

kornilova203 commented Jun 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kornilova203 commented Jun 25, 2018

jonas commented Jun 25, 2018

jonas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kornilova203 commented Jun 28, 2018

kornilova203 commented Jun 28, 2018 • edited Loading

jonas left a comment

Choose a reason for hiding this comment

kornilova203 commented Jun 28, 2018 •

edited

Loading