For c++, Generate enum classes instead of enum from mojom.

For c++, Generate enum classes instead of enum from mojom.

Change the mojom c++ binding generator to produce C++11 style enum
classes instead of just enums.  Fixup generated code to be compatible
with this new (stricter) syntax.

Also, fixup existing code to be compatible with the new syntax
requirements

BUG=
R=viettrungluu@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/87fe514b87c52e09c4c3ffc406c8afb8c068a3f4

[johngro, 2015-10-01 17:05:17]

johngro@google.com changed reviewers:
+ vtl@google.com

[viettrungluu, 2015-10-01 17:49:54]

viettrungluu@chromium.org changed reviewers:
+ viettrungluu@chromium.org

[viettrungluu, 2015-10-01 17:49:56]

https://codereview.chromium.org/1375313006/diff/1/mojo/public/tools/bindings/...
File mojo/public/tools/bindings/generators/cpp_templates/enum_macros.tmpl
(right):

https://codereview.chromium.org/1375313006/diff/1/mojo/public/tools/bindings/...
mojo/public/tools/bindings/generators/cpp_templates/enum_macros.tmpl:50: static
inline std::ostream& operator<<(
You need to include appropriate headers for std::ostream.

It's extremely unfortunate to force the inclusion of the full <ostream> header
for everyone.

Instead, you should not declare/define this inline, and only include <iosfwd>.
(Of course, then you need an out-of-line definition, in the generated .cc file,
which of course will have to include <ostream>.)

https://codereview.chromium.org/1375313006/diff/1/mojo/public/tools/bindings/...
mojo/public/tools/bindings/generators/cpp_templates/enum_macros.tmpl:53: return
(stream << static_cast<typename
std::underlying_type<{{class_name}}::{{enum.name}}>::type>(val));
The underlying type for our enums is always int32_t, so you should skip the
type_traits mumbo-jumbo (and not include <type_traits>).

https://codereview.chromium.org/1375313006/diff/1/mojo/public/tools/bindings/...
File mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl
(right):

https://codereview.chromium.org/1375313006/diff/1/mojo/public/tools/bindings/...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:100:
static_cast<typename std::underlying_type<
Ditto -- no need for underlying_type.

https://codereview.chromium.org/1375313006/diff/1/mojo/public/tools/bindings/...
File
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl
(right):

https://codereview.chromium.org/1375313006/diff/1/mojo/public/tools/bindings/...
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl:83:
static_cast<typename std::underlying_type<
"

https://codereview.chromium.org/1375313006/diff/1/mojo/public/tools/bindings/...
File mojo/public/tools/bindings/generators/mojom_cpp_generator.py (right):

https://codereview.chromium.org/1375313006/diff/1/mojo/public/tools/bindings/...
mojo/public/tools/bindings/generators/mojom_cpp_generator.py:280: #
TODO(johngro) : should this be allowed at all?
Are we using this? Are there test cases?

https://codereview.chromium.org/1375313006/diff/1/mojo/public/tools/bindings/...
mojo/public/tools/bindings/generators/mojom_cpp_generator.py:281: if
IsEnumToken(token) and (None != kind) and (not mojom.IsEnumKind(kind)):
Can |kind| really be None?

Side points:
* Drop the unneeded parentheses.
* |kind is not None|, not |kind != None| (which would still be preferable to
|None != kind|.

https://codereview.chromium.org/1375313006/diff/1/services/view_manager/sched...
File services/view_manager/scheduled_animation_group.cc (right):

https://codereview.chromium.org/1375313006/diff/1/services/view_manager/sched...
services/view_manager/scheduled_animation_group.cc:12: using
mojo::AnimationProperty::NONE;
These ones are now redundant; the single |using mojo::AnimationProperty;| is now
sufficient.

https://codereview.chromium.org/1375313006/diff/1/services/view_manager/view_...
File services/view_manager/view_manager_service_apptest.cc (right):

https://codereview.chromium.org/1375313006/diff/1/services/view_manager/view_...
services/view_manager/view_manager_service_apptest.cc:22: using
mojo::ErrorCode::NONE;
This is not needed.

https://codereview.chromium.org/1375313006/diff/1/services/view_manager/view_...
services/view_manager/view_manager_service_apptest.cc:27: using
mojo::OrderDirection::ABOVE;
Neither are these.

[johngro, 2015-10-02 00:49:04]

top level comment

Since you seem to want to review both the tool changes as well as the results of
the fixup, I am going to abandon the version of this CL which contains only the
tool changes.

https://codereview.chromium.org/1375313006/diff/1/mojo/public/tools/bindings/...
File mojo/public/tools/bindings/generators/cpp_templates/enum_macros.tmpl
(right):

https://codereview.chromium.org/1375313006/diff/1/mojo/public/tools/bindings/...
mojo/public/tools/bindings/generators/cpp_templates/enum_macros.tmpl:50: static
inline std::ostream& operator<<(
On 2015/10/01 17:49:55, viettrungluu wrote:
> You need to include appropriate headers for std::ostream.
> 
> It's extremely unfortunate to force the inclusion of the full <ostream> header
> for everyone.
> 
> Instead, you should not declare/define this inline, and only include <iosfwd>.
> (Of course, then you need an out-of-line definition, in the generated .cc
file,
> which of course will have to include <ostream>.)

Done.

https://codereview.chromium.org/1375313006/diff/1/mojo/public/tools/bindings/...
mojo/public/tools/bindings/generators/cpp_templates/enum_macros.tmpl:53: return
(stream << static_cast<typename
std::underlying_type<{{class_name}}::{{enum.name}}>::type>(val));
On 2015/10/01 17:49:55, viettrungluu wrote:
> The underlying type for our enums is always int32_t, so you should skip the
> type_traits mumbo-jumbo (and not include <type_traits>).

Done.

https://codereview.chromium.org/1375313006/diff/1/mojo/public/tools/bindings/...
File mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl
(right):

https://codereview.chromium.org/1375313006/diff/1/mojo/public/tools/bindings/...
mojo/public/tools/bindings/generators/cpp_templates/struct_macros.tmpl:100:
static_cast<typename std::underlying_type<
On 2015/10/01 17:49:55, viettrungluu wrote:
> Ditto -- no need for underlying_type.

Done.

https://codereview.chromium.org/1375313006/diff/1/mojo/public/tools/bindings/...
File
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl
(right):

https://codereview.chromium.org/1375313006/diff/1/mojo/public/tools/bindings/...
mojo/public/tools/bindings/generators/cpp_templates/union_serialization_definition.tmpl:83:
static_cast<typename std::underlying_type<
On 2015/10/01 17:49:55, viettrungluu wrote:
> "

Done.

https://codereview.chromium.org/1375313006/diff/1/mojo/public/tools/bindings/...
File mojo/public/tools/bindings/generators/mojom_cpp_generator.py (right):

https://codereview.chromium.org/1375313006/diff/1/mojo/public/tools/bindings/...
mojo/public/tools/bindings/generators/mojom_cpp_generator.py:280: #
TODO(johngro) : should this be allowed at all?
On 2015/10/01 17:49:55, viettrungluu wrote:
> Are we using this? Are there test cases?

Unclear, and sort of.

"Are we using this?"
To accomplish useful things?  I don't know.  The code path is exercised by the
build, however.

"Are there test cases?"
There is mojo/public/interfaces/bindings/tests/test_structs.mojom which
exercises the first half of the check when it says..

struct ScopedConstants {
  const int32 TEN = 10;
  const int32 ALSO_TEN = TEN;
  enum EType {
    E0,
    E1,
    E2 = 10,
    E3 = E2,
    E4,
  };
  const int32 TEN_TOO = EType.E2;
  EType f0 = E0; // 0
  EType f1 = E1; // 1
  EType f2 = E2; // 10
  EType f3 = E3; // 10
  EType f4 = E4; // 11
  int32 f5 = TEN;
  int32 f6 = ALSO_TEN;
};

The assignment of the token EType.E2 to the const int32 TEN_TOO results in the
assignment of an enum token (EType.E2) to a non-None, non-Enum kind (int32). 
Generated code requires that a cast be synthesized in this case.

This is currently permitted by the binding generator, and appears to be
permitted by the mojom grammar.  Whether or not it should be is above my pay
grade, which is why I left the question in the comment.

Regarding the second half...  The following statement is considered to be legal
by the current toolchain as well...

struct ScopedConstants {
  ...
  EType f7 = ScopedConstants.TEN;
  ...
};

Since the token (ScopedConstants.TEN) is a named value, and it is of non enum
type, and the kind it is being assigned to is of enum kind, a cast must be
generated.

Note:
  EType f7 = TEN;
will generate an exception.  So will
  const EType blah = Etype.E2;

Similar syntax when dealing with ints, and non-const enum types are permitted,
so this feels like a bug.  For example
  const int32 TEN_TOO = EType.E2; // legal
  const EType TEN_TOO = Etype.E2; // exception
  int32 TEN_TOO = 10; // legal
  EType TEN_TOO = 10; // legal
  int32 TEN_TOO = TEN; // legal
  EType TEN_TOO = TEN; // exception
  int32 TEN_TOO = ScopedConstants.TEN; // legal
  EType TEN_TOO = ScopedConstants.TEN; // legal

So: "Are there tests for this?"
Again sort of.  There is a test for the first half of this check in
test_structs.mojom.  The second half can be legally exercised using the
currently permitted test, but there does not seem to be an explicit test for it.
 Until the question of "should this be allowed at all" is resolved, I am
reluctant to update the tests.  If the answer is yes, then all of the issues
with exceptions when parsing apparently legal syntax need to be resolved and the
tests updated appropriately by the individual who implements the fix.  If the
answer is no, then in addition to updating the toolchain to explicitly disallow
this syntax, different tests need to be written to make sure that the syntax is
enforced.

https://codereview.chromium.org/1375313006/diff/1/mojo/public/tools/bindings/...
mojo/public/tools/bindings/generators/mojom_cpp_generator.py:281: if
IsEnumToken(token) and (None != kind) and (not mojom.IsEnumKind(kind)):
On 2015/10/01 17:49:55, viettrungluu wrote:
> Can |kind| really be None?

Yes.  In addition to the implication of the function defined immediately
following this one as...

def ExpressionToText(value, kind=None):
  return TranslateConstants(value, kind)

There is the common case of assigning a constant value (named or not) to a
member of an enumeration.  In the case that the target is the declaration of a
member of an enumeration (not an instance of one), kind appears to be None.  For
example..

enum MyEnum {
 A = 10,
 B = A,
}

Both result in Kind == none.  There may be other cases, but I am unaware of
them.

 
> Side points:
> * Drop the unneeded parentheses.
> * |kind is not None|, not |kind != None| (which would still be preferable to
> |None != kind|.

done.

https://codereview.chromium.org/1375313006/diff/1/services/view_manager/sched...
File services/view_manager/scheduled_animation_group.cc (right):

https://codereview.chromium.org/1375313006/diff/1/services/view_manager/sched...
services/view_manager/scheduled_animation_group.cc:12: using
mojo::AnimationProperty::NONE;
On 2015/10/01 17:49:55, viettrungluu wrote:
> These ones are now redundant; the single |using mojo::AnimationProperty;| is
now
> sufficient.

Done.

FWIW - this was done by my script, I do not think that I ever layed eyes on this
file.  There may be other instances of this sort of stuff.  Whether or not you
want to track them all down is up to you.

https://codereview.chromium.org/1375313006/diff/1/services/view_manager/view_...
File services/view_manager/view_manager_service_apptest.cc (right):

https://codereview.chromium.org/1375313006/diff/1/services/view_manager/view_...
services/view_manager/view_manager_service_apptest.cc:22: using
mojo::ErrorCode::NONE;
On 2015/10/01 17:49:55, viettrungluu wrote:
> This is not needed.

same response as before

https://codereview.chromium.org/1375313006/diff/1/services/view_manager/view_...
services/view_manager/view_manager_service_apptest.cc:27: using
mojo::OrderDirection::ABOVE;
On 2015/10/01 17:49:55, viettrungluu wrote:
> Neither are these.

"

[viettrungluu, 2015-10-02 01:15:11]

On 2015/10/02 00:49:04, johngro wrote:
> top level comment
> 
> Since you seem to want to review both the tool changes as well as the results
of
> the fixup, I am going to abandon the version of this CL which contains only
the
> tool changes.

sgtm (I actually looked at all the files, at least cursorily, fwiw)

<snip>

https://codereview.chromium.org/1375313006/diff/1/mojo/public/tools/bindings/...
> File mojo/public/tools/bindings/generators/mojom_cpp_generator.py (right):
> 
>
https://codereview.chromium.org/1375313006/diff/1/mojo/public/tools/bindings/...
> mojo/public/tools/bindings/generators/mojom_cpp_generator.py:280: #
> TODO(johngro) : should this be allowed at all?
> On 2015/10/01 17:49:55, viettrungluu wrote:
> > Are we using this? Are there test cases?
> 
> Unclear, and sort of.
> 
> "Are we using this?"
> To accomplish useful things?  I don't know.  The code path is exercised by the
> build, however.
> 
> "Are there test cases?"

Let's make a reasonable decision. I propose:
* If the RHS is typed (i.e., a named value), its type must match the type of the
LHS.
* If the LHS has a non-enum numeric type, its RHS must either be a constant of
the same type or a suitable numeric literal.
* If the LHS is in the definition of an enum, then the RHS may be a either
another enum value (in the same enum -- though there's the problem that you have
to detect cycles!) or a suitable integer literal.
* If the LHS is a use (i.e., const/member/parameter) of an enum, then the RHS
must be an enum value in that enum type.

> There is mojo/public/interfaces/bindings/tests/test_structs.mojom which
> exercises the first half of the check when it says..
> 
> struct ScopedConstants {
>   const int32 TEN = 10;
>   const int32 ALSO_TEN = TEN;
>   enum EType {
>     E0,
>     E1,
>     E2 = 10,
>     E3 = E2,

I'd say these are OK.

>     E4,
>   };
>   const int32 TEN_TOO = EType.E2;

This is not OK.

>   EType f0 = E0; // 0
>   EType f1 = E1; // 1
>   EType f2 = E2; // 10
>   EType f3 = E3; // 10
>   EType f4 = E4; // 11

OK.

>   int32 f5 = TEN;
>   int32 f6 = ALSO_TEN;

Not OK.

> };
> 
> The assignment of the token EType.E2 to the const int32 TEN_TOO results in the
> assignment of an enum token (EType.E2) to a non-None, non-Enum kind (int32). 
> Generated code requires that a cast be synthesized in this case.

Right.

> 
> This is currently permitted by the binding generator, and appears to be
> permitted by the mojom grammar.  Whether or not it should be is above my pay
> grade, which is why I left the question in the comment.

Note that the current generator is quite liberal (sometimes accidentally so)....

> 
> Regarding the second half...  The following statement is considered to be
legal
> by the current toolchain as well...
> 
> struct ScopedConstants {
>   ...
>   EType f7 = ScopedConstants.TEN;
>   ...
> };
> 
> Since the token (ScopedConstants.TEN) is a named value, and it is of non enum
> type, and the kind it is being assigned to is of enum kind, a cast must be
> generated.
> 
> Note:
>   EType f7 = TEN;
> will generate an exception.  So will
>   const EType blah = Etype.E2;
> 
> Similar syntax when dealing with ints, and non-const enum types are permitted,
> so this feels like a bug.  For example
>   const int32 TEN_TOO = EType.E2; // legal
>   const EType TEN_TOO = Etype.E2; // exception
>   int32 TEN_TOO = 10; // legal
>   EType TEN_TOO = 10; // legal
>   int32 TEN_TOO = TEN; // legal
>   EType TEN_TOO = TEN; // exception
>   int32 TEN_TOO = ScopedConstants.TEN; // legal
>   EType TEN_TOO = ScopedConstants.TEN; // legal
> 
> So: "Are there tests for this?"
> Again sort of.  There is a test for the first half of this check in
> test_structs.mojom.  The second half can be legally exercised using the
> currently permitted test, but there does not seem to be an explicit test for
it.
>  Until the question of "should this be allowed at all" is resolved, I am
> reluctant to update the tests.  If the answer is yes, then all of the issues
> with exceptions when parsing apparently legal syntax need to be resolved and
the
> tests updated appropriately by the individual who implements the fix.  If the
> answer is no, then in addition to updating the toolchain to explicitly
disallow
> this syntax, different tests need to be written to make sure that the syntax
is
> enforced.

Currently, enforcement is often non-existent and/or accidental (but a new
generator is being written).

I'd say that if, by removing the "not OK" things from the test mojom files,
everything builds (without the additional casts), then we go with that.

We should, of course, update the spec (and also make it more precise), and make
sure that the new compiler rejects all the not-OK cases.

(So the AI is to try modifying the test .mojom's suitably, and seeing if it
builds. If there are uses of these "not OK" things outside of tests, then the
question becomes of updating those uses.)

> 
>
https://codereview.chromium.org/1375313006/diff/1/mojo/public/tools/bindings/...
> mojo/public/tools/bindings/generators/mojom_cpp_generator.py:281: if
> IsEnumToken(token) and (None != kind) and (not mojom.IsEnumKind(kind)):
> On 2015/10/01 17:49:55, viettrungluu wrote:
> > Can |kind| really be None?
> 
> Yes.  In addition to the implication of the function defined immediately
> following this one as...
> 
> def ExpressionToText(value, kind=None):
>   return TranslateConstants(value, kind)
> 
> There is the common case of assigning a constant value (named or not) to a
> member of an enumeration.  In the case that the target is the declaration of a
> member of an enumeration (not an instance of one), kind appears to be None. 
For
> example..
> 
> enum MyEnum {
>  A = 10,
>  B = A,
> }
> 
> Both result in Kind == none.  There may be other cases, but I am unaware of
> them.

OK.

> 
>  
> > Side points:
> > * Drop the unneeded parentheses.
> > * |kind is not None|, not |kind != None| (which would still be preferable to
> > |None != kind|.
> 
> done.
> 
>
https://codereview.chromium.org/1375313006/diff/1/services/view_manager/sched...
> File services/view_manager/scheduled_animation_group.cc (right):
> 
>
https://codereview.chromium.org/1375313006/diff/1/services/view_manager/sched...
> services/view_manager/scheduled_animation_group.cc:12: using
> mojo::AnimationProperty::NONE;
> On 2015/10/01 17:49:55, viettrungluu wrote:
> > These ones are now redundant; the single |using mojo::AnimationProperty;| is
> now
> > sufficient.
> 
> Done.
> 
> FWIW - this was done by my script, I do not think that I ever layed eyes on
this
> file.  There may be other instances of this sort of stuff.  Whether or not you
> want to track them all down is up to you.

I looked at each file at least briefly. If you want to grep through your diff
for "using", that might be a good idea. (We don't use "using" that much, so
there shouldn't be many occurrences.)

<snip>

[viettrungluu, 2015-10-02 01:19:28]

(I looked at the diff, and there were only these cases of "using".)

https://codereview.chromium.org/1375313006/diff/20001/services/view_manager/a...
File services/view_manager/animation_runner_unittest.cc (right):

https://codereview.chromium.org/1375313006/diff/20001/services/view_manager/a...
services/view_manager/animation_runner_unittest.cc:17: using
mojo::AnimationProperty::NONE;
Delete these too, I guess.

[viettrungluu, 2015-10-02 01:24:44]

/cc rudominer

Apparently, you also have to update
services/native_viewport/platform_viewport_android.cc (and maybe some other
Android-specific files) -- see the red Android trybot results.

Otherwise, LGTM with Android fixed and the few extra "using"s removed.

If it isn't too onerous to remove the need to generate the extra static_casts
(e.g., by modifying the test .mojoms), let's do so. Otherwise, we can consider
that separately.

Mitch -- see the discussion above (about when we allow/should allow "=
MyEnum.VALUE" and "= 123").

[johngro, 2015-10-02 16:13:41]

https://codereview.chromium.org/1375313006/diff/20001/services/view_manager/a...
File services/view_manager/animation_runner_unittest.cc (right):

https://codereview.chromium.org/1375313006/diff/20001/services/view_manager/a...
services/view_manager/animation_runner_unittest.cc:17: using
mojo::AnimationProperty::NONE;
On 2015/10/02 01:19:28, viettrungluu wrote:
> Delete these too, I guess.

Done.

[johngro, 2015-10-02 16:44:48]

> Let's make a reasonable decision. I propose:
> * If the RHS is typed (i.e., a named value), its type must match the type of
the
> LHS.
> * If the LHS has a non-enum numeric type, its RHS must either be a constant of
> the same type or a suitable numeric literal.
> * If the LHS is in the definition of an enum, then the RHS may be a either
> another enum value (in the same enum -- though there's the problem that you
have
> to detect cycles!) or a suitable integer literal.
> * If the LHS is a use (i.e., const/member/parameter) of an enum, then the RHS
> must be an enum value in that enum type.

 
> > struct ScopedConstants {
> >   const int32 TEN = 10;
> >   const int32 ALSO_TEN = TEN;

<snip>

> 
> >   int32 f5 = TEN;
> >   int32 f6 = ALSO_TEN;
> 
> Not OK.

I think that this should be allowed.  TEN and ALSO_TEN are both named values
with the type int32.  The LHS of this expression is also an int32.  There should
be no problem here.

furthermore, by your first rule above..

struct MyStruct {
  enum MyEnum { A, B, C, D };
  const MyEnum MY_CONST_ENUM = MyEnum.A;
  MyEnum my_var_enum = MY_CONST_ENUM;
};

... should also be allowed.  The only reason that this does not work today is
that the const MyEnum crashes the generator.  According to the current grammer,
const Enum's are disallowed; const assignable types must be either strings or
BasicTypes (ints, bools and floats), so this is probably not something we need
to handle unless we decide that const enum symbols are OK.  While I find the
lack of symmetry here to be a bit odd, I would need to stretch to come up with
some reason that I would want to do this, so I suppose its no big deal.

Applying your rules to some more examples, it seems like we have...

enum EType {
  E1,       // legal, default initialization
  E2 = 10,  // legal, ints may be used to initialize enum members
  E3 = E2,  // legal, members of an enum may be used to initialize other members
  E4 = OtherEType.VAL, // illegal, cannot use values from other enums
  E5 = CONST_INT, // legal, RHS is an integer named value
  E6 = "foo", // illegal, this is not C#.
  E7 = 6.4,   // illegal.
  E8 = 6.0,   // still illegal.
  E9 = 0x100000000, // illegal, cannot convert literal to int without loss of
                    // precision.
};

const int32 TEN = 10;         // legal; int literal assigned to const int
const int32 TEN_TOO = TEN;    // legal; int named value assigned to const int
const int32 TEN_E = EType.E2; // illegal, may not assign an enum to an int.
const EType ETEN  = Etype.E2; // illegal; const enums are not allowed.

int32 TEN_TOO = 10;       // legal
int32 TEN_TOO = TEN;      // legal
int32 TEN_TOO = EType.E2; // illegal (but currently works)
int32 TEN_TOO = ETEN;     // illegal

EType TEN_TOO = 10;   // illegal, may not assign an int to an enum var
EType TEN_TOO = TEN;  // still illegal
EType TEN_TOO = ScopedConstants.TEN; // still illegal (but works currently)
EType TEN_TOO = EType.E2; // legal
EType TEN_TOO = ETEN; // would be leagal, but there is no way to make a named
                      // value enum.

[johngro, 2015-10-02 17:11:25]

On 2015/10/02 01:24:44, viettrungluu wrote:

> If it isn't too onerous to remove the need to generate the extra static_casts
> (e.g., by modifying the test .mojoms), let's do so. Otherwise, we can consider
> that separately.

ok, per our f2f discussion, I'm going to land this as is, and immediately follow
up with a CL which explicitly disallows this behavior in the C++ mojom generator
(removing the tests which test this as I do).

This is a bit sloppy, as the existing grammar still allows this, and because the
enforcement is only in the C++ generator (bad behavior will not be caught if you
tell the generator to create only the Dart bindings).  Given that this is
completely re-worked, I'm not too concerned.  I expect that the spec and tests
will be updated when the generator is re-done.

[johngro, 2015-10-02 17:12:16]

Committed patchset #3 (id:40001) manually as
87fe514b87c52e09c4c3ffc406c8afb8c068a3f4.