[libcamera-devel,v2,5/7] test: generated_serializer: Test Flags that is struct member
diff mbox series

Message ID 20220818064923.2573060-6-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • utils: ipc: Add support for enums and Flags
Related show

Commit Message

Paul Elder Aug. 18, 2022, 6:49 a.m. UTC 
Add a Flags field to the test struct to test
serialization/deserialization of Flags that are struct members.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
Changes in v2:
- use new attribute-based mojom definition for Flags
---
 .../generated_serializer_test.cpp             | 19 +++++++++++++++++++
 .../include/libcamera/ipa/test.mojom          |  8 ++++++++
 2 files changed, 27 insertions(+)

Comments

Laurent Pinchart Aug. 19, 2022, 12:52 a.m. UTC | #1 
Hi Paul,

Thank you for the patch.

On Thu, Aug 18, 2022 at 03:49:20PM +0900, Paul Elder via libcamera-devel wrote:
> Add a Flags field to the test struct to test
> serialization/deserialization of Flags that are struct members.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> Changes in v2:
> - use new attribute-based mojom definition for Flags
> ---
>  .../generated_serializer_test.cpp             | 19 +++++++++++++++++++
>  .../include/libcamera/ipa/test.mojom          |  8 ++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/test/serialization/generated_serializer/generated_serializer_test.cpp b/test/serialization/generated_serializer/generated_serializer_test.cpp
> index a4639a80..a2d71d62 100644
> --- a/test/serialization/generated_serializer/generated_serializer_test.cpp
> +++ b/test/serialization/generated_serializer/generated_serializer_test.cpp
> @@ -53,6 +53,11 @@ if (struct1.field != struct2.field) {				\
>  		t.i  = 58527;
>  		t.c = ipa::test::IPAOperationInit;
>  
> +		Flags<ipa::test::ErrorFlags> flags;
> +		flags |= ipa::test::ErrorFlags::Error1;
> +		flags |= ipa::test::ErrorFlags::Error2;
> +		t.f = flags;
> +
>  		std::vector<uint8_t> serialized;
>  
>  		std::tie(serialized, ignore) =
> @@ -72,6 +77,10 @@ if (struct1.field != struct2.field) {				\
>  		TEST_FIELD_EQUALITY(t, u, i);
>  		TEST_FIELD_EQUALITY(t, u, c);
>  
> +		if (t.f != u.f) {
> +			cerr << "Flags f field incorrect" << endl;
> +			return TestFail;
> +		}
>  
>  		/* Test vector of generated structs */
>  		std::vector<ipa::test::TestStruct> v = { t, u };
> @@ -96,12 +105,22 @@ if (struct1.field != struct2.field) {				\
>  		TEST_FIELD_EQUALITY(v[0], w[0], i);
>  		TEST_FIELD_EQUALITY(v[0], w[0], c);
>  
> +		if (v[0].f != w[0].f) {
> +			cerr << "Flags f field incorrect" << endl;
> +			return TestFail;
> +		}
> +
>  		TEST_FIELD_EQUALITY(v[1], w[1], s1);
>  		TEST_FIELD_EQUALITY(v[1], w[1], s2);
>  		TEST_FIELD_EQUALITY(v[1], w[1], s3);
>  		TEST_FIELD_EQUALITY(v[1], w[1], i);
>  		TEST_FIELD_EQUALITY(v[1], w[1], c);
>  
> +		if (v[1].f != w[1].f) {
> +			cerr << "Flags f field incorrect" << endl;
> +			return TestFail;
> +		}
> +
>  		return TestPass;
>  	}
>  
> diff --git a/test/serialization/generated_serializer/include/libcamera/ipa/test.mojom b/test/serialization/generated_serializer/include/libcamera/ipa/test.mojom
> index 73081b40..698f4e89 100644
> --- a/test/serialization/generated_serializer/include/libcamera/ipa/test.mojom
> +++ b/test/serialization/generated_serializer/include/libcamera/ipa/test.mojom
> @@ -9,6 +9,13 @@ enum IPAOperationCode {
>  	IPAOperationStop,
>  };
>  
> +enum ErrorFlags {

I assume this will become

[scopedEnum] enum ErrorFlags {

> +	Error1 = 0x1,
> +	Error2 = 0x2,
> +	Error3 = 0x4,
> +	Error4 = 0x8,
> +};
> +
>  struct IPASettings {};
>  
>  struct TestStruct {
> @@ -19,6 +26,7 @@ struct TestStruct {
>  	int32 i;
>  	string s3;
>  	IPAOperationCode c;
> +	[Flags] ErrorFlags f;

I forgot to mention that in the review of 4/7, so I'll mention it here:
should the attribute be [flags] to match the camelCase syntax of other
attributes ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  };
>  
>  interface IPATestInterface {
Nicolas Dufresne via libcamera-devel Aug. 19, 2022, 7:43 a.m. UTC | #2 
Hi Laurent,

On Fri, Aug 19, 2022 at 03:52:34AM +0300, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.
> 
> On Thu, Aug 18, 2022 at 03:49:20PM +0900, Paul Elder via libcamera-devel wrote:
> > Add a Flags field to the test struct to test
> > serialization/deserialization of Flags that are struct members.
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > 
> > ---
> > Changes in v2:
> > - use new attribute-based mojom definition for Flags
> > ---
> >  .../generated_serializer_test.cpp             | 19 +++++++++++++++++++
> >  .../include/libcamera/ipa/test.mojom          |  8 ++++++++
> >  2 files changed, 27 insertions(+)
> > 
> > diff --git a/test/serialization/generated_serializer/generated_serializer_test.cpp b/test/serialization/generated_serializer/generated_serializer_test.cpp
> > index a4639a80..a2d71d62 100644
> > --- a/test/serialization/generated_serializer/generated_serializer_test.cpp
> > +++ b/test/serialization/generated_serializer/generated_serializer_test.cpp
> > @@ -53,6 +53,11 @@ if (struct1.field != struct2.field) {				\
> >  		t.i  = 58527;
> >  		t.c = ipa::test::IPAOperationInit;
> >  
> > +		Flags<ipa::test::ErrorFlags> flags;
> > +		flags |= ipa::test::ErrorFlags::Error1;
> > +		flags |= ipa::test::ErrorFlags::Error2;
> > +		t.f = flags;
> > +
> >  		std::vector<uint8_t> serialized;
> >  
> >  		std::tie(serialized, ignore) =
> > @@ -72,6 +77,10 @@ if (struct1.field != struct2.field) {				\
> >  		TEST_FIELD_EQUALITY(t, u, i);
> >  		TEST_FIELD_EQUALITY(t, u, c);
> >  
> > +		if (t.f != u.f) {
> > +			cerr << "Flags f field incorrect" << endl;
> > +			return TestFail;
> > +		}
> >  
> >  		/* Test vector of generated structs */
> >  		std::vector<ipa::test::TestStruct> v = { t, u };
> > @@ -96,12 +105,22 @@ if (struct1.field != struct2.field) {				\
> >  		TEST_FIELD_EQUALITY(v[0], w[0], i);
> >  		TEST_FIELD_EQUALITY(v[0], w[0], c);
> >  
> > +		if (v[0].f != w[0].f) {
> > +			cerr << "Flags f field incorrect" << endl;
> > +			return TestFail;
> > +		}
> > +
> >  		TEST_FIELD_EQUALITY(v[1], w[1], s1);
> >  		TEST_FIELD_EQUALITY(v[1], w[1], s2);
> >  		TEST_FIELD_EQUALITY(v[1], w[1], s3);
> >  		TEST_FIELD_EQUALITY(v[1], w[1], i);
> >  		TEST_FIELD_EQUALITY(v[1], w[1], c);
> >  
> > +		if (v[1].f != w[1].f) {
> > +			cerr << "Flags f field incorrect" << endl;
> > +			return TestFail;
> > +		}
> > +
> >  		return TestPass;
> >  	}
> >  
> > diff --git a/test/serialization/generated_serializer/include/libcamera/ipa/test.mojom b/test/serialization/generated_serializer/include/libcamera/ipa/test.mojom
> > index 73081b40..698f4e89 100644
> > --- a/test/serialization/generated_serializer/include/libcamera/ipa/test.mojom
> > +++ b/test/serialization/generated_serializer/include/libcamera/ipa/test.mojom
> > @@ -9,6 +9,13 @@ enum IPAOperationCode {
> >  	IPAOperationStop,
> >  };
> >  
> > +enum ErrorFlags {
> 
> I assume this will become
> 
> [scopedEnum] enum ErrorFlags {

Yeah, it will.

> 
> > +	Error1 = 0x1,
> > +	Error2 = 0x2,
> > +	Error3 = 0x4,
> > +	Error4 = 0x8,
> > +};
> > +
> >  struct IPASettings {};
> >  
> >  struct TestStruct {
> > @@ -19,6 +26,7 @@ struct TestStruct {
> >  	int32 i;
> >  	string s3;
> >  	IPAOperationCode c;
> > +	[Flags] ErrorFlags f;
> 
> I forgot to mention that in the review of 4/7, so I'll mention it here:
> should the attribute be [flags] to match the camelCase syntax of other
> attributes ?

That's what I thought too on one hand, but my other hand said that it's
meant to become Flags<ErrorFlags>, so I matched the capitalization to
that.

Which hand is better?

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>


Thanks,

Paul

> 
> >  };
> >  
> >  interface IPATestInterface {
Laurent Pinchart Aug. 19, 2022, 8:07 a.m. UTC | #3 
Hi Paul,

On Fri, Aug 19, 2022 at 04:43:02PM +0900, paul.elder@ideasonboard.com wrote:
> On Fri, Aug 19, 2022 at 03:52:34AM +0300, Laurent Pinchart wrote:
> > On Thu, Aug 18, 2022 at 03:49:20PM +0900, Paul Elder via libcamera-devel wrote:
> > > Add a Flags field to the test struct to test
> > > serialization/deserialization of Flags that are struct members.
> > > 
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > 
> > > ---
> > > Changes in v2:
> > > - use new attribute-based mojom definition for Flags
> > > ---
> > >  .../generated_serializer_test.cpp             | 19 +++++++++++++++++++
> > >  .../include/libcamera/ipa/test.mojom          |  8 ++++++++
> > >  2 files changed, 27 insertions(+)
> > > 
> > > diff --git a/test/serialization/generated_serializer/generated_serializer_test.cpp b/test/serialization/generated_serializer/generated_serializer_test.cpp
> > > index a4639a80..a2d71d62 100644
> > > --- a/test/serialization/generated_serializer/generated_serializer_test.cpp
> > > +++ b/test/serialization/generated_serializer/generated_serializer_test.cpp
> > > @@ -53,6 +53,11 @@ if (struct1.field != struct2.field) {				\
> > >  		t.i  = 58527;
> > >  		t.c = ipa::test::IPAOperationInit;
> > >  
> > > +		Flags<ipa::test::ErrorFlags> flags;
> > > +		flags |= ipa::test::ErrorFlags::Error1;
> > > +		flags |= ipa::test::ErrorFlags::Error2;
> > > +		t.f = flags;
> > > +
> > >  		std::vector<uint8_t> serialized;
> > >  
> > >  		std::tie(serialized, ignore) =
> > > @@ -72,6 +77,10 @@ if (struct1.field != struct2.field) {				\
> > >  		TEST_FIELD_EQUALITY(t, u, i);
> > >  		TEST_FIELD_EQUALITY(t, u, c);
> > >  
> > > +		if (t.f != u.f) {
> > > +			cerr << "Flags f field incorrect" << endl;
> > > +			return TestFail;
> > > +		}
> > >  
> > >  		/* Test vector of generated structs */
> > >  		std::vector<ipa::test::TestStruct> v = { t, u };
> > > @@ -96,12 +105,22 @@ if (struct1.field != struct2.field) {				\
> > >  		TEST_FIELD_EQUALITY(v[0], w[0], i);
> > >  		TEST_FIELD_EQUALITY(v[0], w[0], c);
> > >  
> > > +		if (v[0].f != w[0].f) {
> > > +			cerr << "Flags f field incorrect" << endl;
> > > +			return TestFail;
> > > +		}
> > > +
> > >  		TEST_FIELD_EQUALITY(v[1], w[1], s1);
> > >  		TEST_FIELD_EQUALITY(v[1], w[1], s2);
> > >  		TEST_FIELD_EQUALITY(v[1], w[1], s3);
> > >  		TEST_FIELD_EQUALITY(v[1], w[1], i);
> > >  		TEST_FIELD_EQUALITY(v[1], w[1], c);
> > >  
> > > +		if (v[1].f != w[1].f) {
> > > +			cerr << "Flags f field incorrect" << endl;
> > > +			return TestFail;
> > > +		}
> > > +
> > >  		return TestPass;
> > >  	}
> > >  
> > > diff --git a/test/serialization/generated_serializer/include/libcamera/ipa/test.mojom b/test/serialization/generated_serializer/include/libcamera/ipa/test.mojom
> > > index 73081b40..698f4e89 100644
> > > --- a/test/serialization/generated_serializer/include/libcamera/ipa/test.mojom
> > > +++ b/test/serialization/generated_serializer/include/libcamera/ipa/test.mojom
> > > @@ -9,6 +9,13 @@ enum IPAOperationCode {
> > >  	IPAOperationStop,
> > >  };
> > >  
> > > +enum ErrorFlags {
> > 
> > I assume this will become
> > 
> > [scopedEnum] enum ErrorFlags {
> 
> Yeah, it will.
> 
> > 
> > > +	Error1 = 0x1,
> > > +	Error2 = 0x2,
> > > +	Error3 = 0x4,
> > > +	Error4 = 0x8,
> > > +};
> > > +
> > >  struct IPASettings {};
> > >  
> > >  struct TestStruct {
> > > @@ -19,6 +26,7 @@ struct TestStruct {
> > >  	int32 i;
> > >  	string s3;
> > >  	IPAOperationCode c;
> > > +	[Flags] ErrorFlags f;
> > 
> > I forgot to mention that in the review of 4/7, so I'll mention it here:
> > should the attribute be [flags] to match the camelCase syntax of other
> > attributes ?
> 
> That's what I thought too on one hand, but my other hand said that it's
> meant to become Flags<ErrorFlags>, so I matched the capitalization to
> that.
> 
> Which hand is better?

I usually like consistency, but I'm ok with Flags if you strongly prefer
that or if there's a general mild preference.

> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > >  };
> > >  
> > >  interface IPATestInterface {

Patch
diff mbox series 

diff --git a/test/serialization/generated_serializer/generated_serializer_test.cpp b/test/serialization/generated_serializer/generated_serializer_test.cpp
index a4639a80..a2d71d62 100644
--- a/test/serialization/generated_serializer/generated_serializer_test.cpp
+++ b/test/serialization/generated_serializer/generated_serializer_test.cpp
@@ -53,6 +53,11 @@  if (struct1.field != struct2.field) {				\
 		t.i  = 58527;
 		t.c = ipa::test::IPAOperationInit;
 
+		Flags<ipa::test::ErrorFlags> flags;
+		flags |= ipa::test::ErrorFlags::Error1;
+		flags |= ipa::test::ErrorFlags::Error2;
+		t.f = flags;
+
 		std::vector<uint8_t> serialized;
 
 		std::tie(serialized, ignore) =
@@ -72,6 +77,10 @@  if (struct1.field != struct2.field) {				\
 		TEST_FIELD_EQUALITY(t, u, i);
 		TEST_FIELD_EQUALITY(t, u, c);
 
+		if (t.f != u.f) {
+			cerr << "Flags f field incorrect" << endl;
+			return TestFail;
+		}
 
 		/* Test vector of generated structs */
 		std::vector<ipa::test::TestStruct> v = { t, u };
@@ -96,12 +105,22 @@  if (struct1.field != struct2.field) {				\
 		TEST_FIELD_EQUALITY(v[0], w[0], i);
 		TEST_FIELD_EQUALITY(v[0], w[0], c);
 
+		if (v[0].f != w[0].f) {
+			cerr << "Flags f field incorrect" << endl;
+			return TestFail;
+		}
+
 		TEST_FIELD_EQUALITY(v[1], w[1], s1);
 		TEST_FIELD_EQUALITY(v[1], w[1], s2);
 		TEST_FIELD_EQUALITY(v[1], w[1], s3);
 		TEST_FIELD_EQUALITY(v[1], w[1], i);
 		TEST_FIELD_EQUALITY(v[1], w[1], c);
 
+		if (v[1].f != w[1].f) {
+			cerr << "Flags f field incorrect" << endl;
+			return TestFail;
+		}
+
 		return TestPass;
 	}
 
diff --git a/test/serialization/generated_serializer/include/libcamera/ipa/test.mojom b/test/serialization/generated_serializer/include/libcamera/ipa/test.mojom
index 73081b40..698f4e89 100644
--- a/test/serialization/generated_serializer/include/libcamera/ipa/test.mojom
+++ b/test/serialization/generated_serializer/include/libcamera/ipa/test.mojom
@@ -9,6 +9,13 @@  enum IPAOperationCode {
 	IPAOperationStop,
 };
 
+enum ErrorFlags {
+	Error1 = 0x1,
+	Error2 = 0x2,
+	Error3 = 0x4,
+	Error4 = 0x8,
+};
+
 struct IPASettings {};
 
 struct TestStruct {
@@ -19,6 +26,7 @@  struct TestStruct {
 	int32 i;
 	string s3;
 	IPAOperationCode c;
+	[Flags] ErrorFlags f;
 };
 
 interface IPATestInterface {