Implement IDL enum feature #110

hwanseung · 2017-10-06T12:27:58Z

Implement IDL enum feature. Some part of codes should be generated
automatically at another files but this patch doesn't do that yet.
So, it will be finished in follow-up patch.

ISSUE=#80

hwanseung · 2017-10-06T12:30:10Z

core/idl_types.h

@@ -25,5 +25,8 @@ struct IDLLongLong final : public IDLBaseHelper<int64_t> {};
 struct IDLLong final : public IDLBaseHelper<int32_t> {};
 struct IDLShort final : public IDLBaseHelper<int16_t> {};
 struct IDLString final : public IDLBaseHelper<std::string> {};
+// FIXME(Hwansung): should be generated automatically


i think this lines should be generated automatically at another files.

I think so too ! should be generated automatically ! and another files !

hwanseung · 2017-10-06T12:30:17Z

core/native_type_traits.h

@@ -133,4 +133,34 @@ struct NativeTypeTraits<IDLString> : public NativeTypeTraitsBase<IDLString> {
  }
 };

+// FIXME(Hwansung): should be generated automatically


i think this lines should be generated automatically at another files.

I thinks so too also

yjaeseok · 2017-10-06T12:45:40Z

core/native_type_traits.h

@@ -133,4 +133,34 @@ struct NativeTypeTraits<IDLString> : public NativeTypeTraitsBase<IDLString> {
  }
 };

+// FIXME(Hwansung): should be generated automatically


I thinks so too also

yjaeseok · 2017-10-06T12:50:34Z

core/native_type_traits.h

+        .ThrowAsJavaScriptException();
+    return OperationType::NONE;
+  }
+};


If my PR (#108) is merged before this PR,
This Type must be implement IsTypeEquals methods like below :

static bool IsTypeEquals(const Napi::Value& js_value) { return js_value.IsOperationType(); }

otherwise, i should implement this method! :)

okay i will rebase it.
unfortunately we should implement more things in IsTypeEquals function instead of to use js_value.IsOperationType().
because js_value will be string type in this case. and it should be check that value can be Enum type.

If IsOperationType() implementation is too large to apply this PR, I think the implementation can be divided another PR.

Other things LGTM.

If you complete implement ationIsOperationType() .

you can check argument first, and yiu can return value.

please refer to below code :

template <> struct NativeTypeTraits<IDLOperationType> : public NativeTypeTraitsBase<IDLOperationType> { static OperationType NativeValue(const Napi::Env& env, const Napi::Value& js_value) { if (IsTypeEquals(js_value)) { std::string value = js_value.ToString().Utf8Value(); if (value.compare("add") == 0) { return OperationType::ADD; } else if (value.compare("sub") == 0) { return OperationType::SUB; } else if (value.compare("mul") == 0) { return OperationType::MUL; } else if (value.compare("div") == 0) { return OperationType::DIV; } } else { Napi::TypeError::New(env, "It's an invalid string.") .ThrowAsJavaScriptException(); return OperationType::NONE; } };

romandev · 2017-10-07T04:14:02Z

IMHO, the IDL enum type is just string type in Javascript.
On the other hand, enum type in C++ is integer type.
So, it might be unexpected behavior in JS developer side.

For example,

// In IDL
enum OperationType {
  "add",
  "sub",
  "mul",
  "div"
};

// In Javascript
// JS developer just passes string.
calculator('add', 10, 20);

// In C++
double Calculator::Calculate(const std::string& operator, double number1, double number2) {
  // The following function can take only "add", "sub", "mul", and "div".
  CalculateInternal(operator, number1, number2);
}

// If the enum string is converted to C++ enum in binding side,
// C++ developer might need to convert it again.
double Calculator::Calculate(OperationType opType, double number1, double number2) {
  // The following function can take only "add", "sub", "mul", and "div".
  CalculateInternal(ToString(opType), number1, number2);
}

WDYT?

hwanseung · 2017-10-08T03:04:09Z

@romandev
did you mean we don't have to make enum type automatically and provide it to user?
and is it role of cpp user to convert string to enum?

i had wanted to provide automatically converting both side.
actually i am not sure what is more convenient and easy to understand to user.
but your suggestion seem like more easy to understand.
so i will change this patch to your suggestion.

romandev · 2017-10-08T05:08:52Z

@hwanseung Hmm, we can discuss about this in Slack in Korean :)

hwanseung · 2017-10-08T08:59:59Z

i changed it to pass string type instead of enum type.

romandev

Okay, I left some comments but lgtm.
Please follow my comments in follow-up CL.

romandev · 2017-10-08T10:21:57Z

core/native_type_traits.h

@@ -160,4 +160,43 @@ struct NativeTypeTraits<IDLString> : public NativeTypeTraitsBase<IDLString> {
  }
 };

+// FIXME(Hwansung): should be generated automatically in another file.


Okay, for now as-is okay. But this is not exact NativeTypeTraits because the IDL enum type will always be string type in c++ side.

romandev · 2017-10-08T10:22:34Z

examples/calculator.js

@@ -144,3 +144,24 @@ describe('generate one more bridge classes from one more interfaces', () => {
    assert.equal(ternary_calculator.add(1, 2, 3), 1 + 2 + 3);
  });
 });
+
+describe('enum type test', () => {


Please rewrite a test in test/ directory and please use jest.

romandev · 2017-10-08T10:22:51Z

examples/calculator.cc

@@ -60,3 +60,18 @@ double Calculator::Div(double number1, double number2) {
 bool Calculator::IsEquals(int16_t number1, int16_t number2) {
  return number1 == number2;
 }
+
+double Calculator::Calculate(const std::string& operatorStr,


Please rewrite a test in test/ directory and please use jest.

Implement IDL enum feature. Some part of codes should be generated automatically at another files but this patch doesn't do that yet. So, it will be finished in follow-up patch. ISSUE=#80

hwanseung added the enhancement label Oct 6, 2017

hwanseung self-assigned this Oct 6, 2017

hwanseung requested review from romandev and yjaeseok October 6, 2017 12:27

hwanseung commented Oct 6, 2017

View reviewed changes

hwanseung force-pushed the enum branch 2 times, most recently from 9f27ce3 to 260692b Compare October 6, 2017 12:41

yjaeseok reviewed Oct 6, 2017

View reviewed changes

implement enum

63bc717

hwanseung force-pushed the enum branch from 260692b to 63bc717 Compare October 8, 2017 08:57

romandev changed the title ~~Implement Enum feature.~~ Implement IDL enum feature. Oct 8, 2017

romandev changed the title ~~Implement IDL enum feature.~~ Implement IDL enum feature Oct 8, 2017

romandev approved these changes Oct 8, 2017

View reviewed changes

romandev merged commit aaa4167 into lunchclass:master Oct 8, 2017

hwanseung deleted the enum branch October 8, 2017 12:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement IDL enum feature #110

Implement IDL enum feature #110

hwanseung commented Oct 6, 2017 •

edited by romandev

Loading

hwanseung Oct 6, 2017

yjaeseok Oct 6, 2017

hwanseung Oct 6, 2017

yjaeseok Oct 6, 2017

yjaeseok Oct 6, 2017

yjaeseok Oct 6, 2017

hwanseung Oct 6, 2017

yjaeseok Oct 6, 2017

yjaeseok Oct 6, 2017 •

edited

Loading

romandev commented Oct 7, 2017

hwanseung commented Oct 8, 2017

romandev commented Oct 8, 2017

hwanseung commented Oct 8, 2017

romandev left a comment

romandev Oct 8, 2017

romandev Oct 8, 2017

romandev Oct 8, 2017

Implement IDL enum feature #110

Implement IDL enum feature #110

Conversation

hwanseung commented Oct 6, 2017 • edited by romandev Loading

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

yjaeseok Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

romandev commented Oct 7, 2017

hwanseung commented Oct 8, 2017

romandev commented Oct 8, 2017

hwanseung commented Oct 8, 2017

romandev 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

hwanseung commented Oct 6, 2017 •

edited by romandev

Loading

yjaeseok Oct 6, 2017 •

edited

Loading