| Message ID | 20181221005329.13597-1-niklas.soderlund@ragnatech.se |
|---|---|
| State | Accepted |
| Commit | 53b549b63158c64a2f8764fcba8bf049fb1cb397 |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thank you for the patch. On Friday, 21 December 2018 02:53:29 EET Niklas Söderlund wrote: > Calling the cleanup() function in the base class Test destructor only > calls the base class empty cleanup() function, not the overloaded one. > This results in tests not cleaning up after themself. Solve this by > explicitly calling the cleanup() function from execute(). > > This was discovered while running valgrind on tests where objects where > allocated in init() and freed in cleanup(). My bad :-/ Destructors must never call virtual methods, as during the execution of a destructor the object behaves as if its dynamic type was identical to the static type of the destructor. The reason is simple : destructors are called in sequence from the leaf class to the root class. The Test destructor thus can access any member of a derive class, as it will already have been destructed. > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > test/test.cpp | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/test/test.cpp b/test/test.cpp > index 4e7779e750d56687..1bb6ebcb9e8acf18 100644 > --- a/test/test.cpp > +++ b/test/test.cpp > @@ -13,7 +13,6 @@ Test::Test() > > Test::~Test() > { > - cleanup(); > } > > int Test::execute() > @@ -24,5 +23,9 @@ int Test::execute() > if (ret < 0) > return ret; > > - return run(); > + ret = run(); > + > + cleanup(); > + > + return ret; > }
Hi Niklas, On Fri, Dec 21, 2018 at 01:53:29AM +0100, Niklas Söderlund wrote: > Calling the cleanup() function in the base class Test destructor only > calls the base class empty cleanup() function, not the overloaded one. > This results in tests not cleaning up after themself. Solve this by > explicitly calling the cleanup() function from execute(). > > This was discovered while running valgrind on tests where objects where > allocated in init() and freed in cleanup(). > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > test/test.cpp | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/test/test.cpp b/test/test.cpp > index 4e7779e750d56687..1bb6ebcb9e8acf18 100644 > --- a/test/test.cpp > +++ b/test/test.cpp > @@ -13,7 +13,6 @@ Test::Test() > > Test::~Test() > { > - cleanup(); > } > > int Test::execute() > @@ -24,5 +23,9 @@ int Test::execute() > if (ret < 0) Shouldn't the same be done here? > return ret; > > - return run(); > + ret = run(); > + > + cleanup(); > + > + return ret; > } > -- > 2.20.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Friday, 21 December 2018 18:26:42 EET jacopo mondi wrote: > On Fri, Dec 21, 2018 at 01:53:29AM +0100, Niklas Söderlund wrote: > > Calling the cleanup() function in the base class Test destructor only > > calls the base class empty cleanup() function, not the overloaded one. > > This results in tests not cleaning up after themself. Solve this by > > explicitly calling the cleanup() function from execute(). > > > > This was discovered while running valgrind on tests where objects where > > allocated in init() and freed in cleanup(). > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > > > test/test.cpp | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/test/test.cpp b/test/test.cpp > > index 4e7779e750d56687..1bb6ebcb9e8acf18 100644 > > --- a/test/test.cpp > > +++ b/test/test.cpp > > @@ -13,7 +13,6 @@ Test::Test() > > Test::~Test() > > { > > - cleanup(); > > } > > > > int Test::execute() > > @@ -24,5 +23,9 @@ int Test::execute() > > if (ret < 0) > > Shouldn't the same be done here? It's a good question. Shouldn't the init() method clean after itself if it fails ? What's your opinion on this generally ? > > return ret; > > > > - return run(); > > + ret = run(); > > + > > + cleanup(); > > + > > + return ret; > > }
Hi Jacopo, Laurent, On 2018-12-22 12:34:22 +0200, Laurent Pinchart wrote: > Hi Jacopo, > > On Friday, 21 December 2018 18:26:42 EET jacopo mondi wrote: > > On Fri, Dec 21, 2018 at 01:53:29AM +0100, Niklas Söderlund wrote: > > > Calling the cleanup() function in the base class Test destructor only > > > calls the base class empty cleanup() function, not the overloaded one. > > > This results in tests not cleaning up after themself. Solve this by > > > explicitly calling the cleanup() function from execute(). > > > > > > This was discovered while running valgrind on tests where objects where > > > allocated in init() and freed in cleanup(). > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > --- > > > > > > test/test.cpp | 7 +++++-- > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > diff --git a/test/test.cpp b/test/test.cpp > > > index 4e7779e750d56687..1bb6ebcb9e8acf18 100644 > > > --- a/test/test.cpp > > > +++ b/test/test.cpp > > > @@ -13,7 +13,6 @@ Test::Test() > > > Test::~Test() > > > { > > > - cleanup(); > > > } > > > > > > int Test::execute() > > > @@ -24,5 +23,9 @@ int Test::execute() > > > if (ret < 0) > > > > Shouldn't the same be done here? > > It's a good question. Shouldn't the init() method clean after itself if it > fails ? What's your opinion on this generally ? My view is that in a perfect world cleanup() should be called here and each test cases implementation of cleanup() should be prepared to handle partially initialized state. Even better in a super perfect world init() and run() would never fail so there would be no need to check the return codes or write the test to begin with as it would never fail anyhow ;-P As we live in an in-perfect world I see little point in calling or not calling cleanup() here, just bail out and make sure it's clear the test failed. If the state created by a half run init()/cealnup() function crashes my machine it's a inconvenience but it would probably motivate me to find and fix the issue quicker :-) > > > > return ret; > > > > > > - return run(); > > > + ret = run(); > > > + > > > + cleanup(); > > > + > > > + return ret; > > > } > > -- > Regards, > > Laurent Pinchart > > >
Hi Laurent, Niklas, On Sat, Dec 22, 2018 at 03:44:10PM +0100, Niklas S?derlund wrote: > Hi Jacopo, Laurent, > > On 2018-12-22 12:34:22 +0200, Laurent Pinchart wrote: > > Hi Jacopo, > > > > On Friday, 21 December 2018 18:26:42 EET jacopo mondi wrote: > > > On Fri, Dec 21, 2018 at 01:53:29AM +0100, Niklas Söderlund wrote: > > > > Calling the cleanup() function in the base class Test destructor only > > > > calls the base class empty cleanup() function, not the overloaded one. > > > > This results in tests not cleaning up after themself. Solve this by > > > > explicitly calling the cleanup() function from execute(). > > > > > > > > This was discovered while running valgrind on tests where objects where > > > > allocated in init() and freed in cleanup(). > > > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > --- > > > > > > > > test/test.cpp | 7 +++++-- > > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/test/test.cpp b/test/test.cpp > > > > index 4e7779e750d56687..1bb6ebcb9e8acf18 100644 > > > > --- a/test/test.cpp > > > > +++ b/test/test.cpp > > > > @@ -13,7 +13,6 @@ Test::Test() > > > > Test::~Test() > > > > { > > > > - cleanup(); > > > > } > > > > > > > > int Test::execute() > > > > @@ -24,5 +23,9 @@ int Test::execute() > > > > if (ret < 0) > > > > > > Shouldn't the same be done here? > > > > It's a good question. Shouldn't the init() method clean after itself if it > > fails ? What's your opinion on this generally ? > > My view is that in a perfect world cleanup() should be called here and > each test cases implementation of cleanup() should be prepared to handle > partially initialized state. Even better in a super perfect world init() > and run() would never fail so there would be no need to check the return > codes or write the test to begin with as it would never fail anyhow ;-P > > As we live in an in-perfect world I see little point in calling or not > calling cleanup() here, just bail out and make sure it's clear the test > failed. If the state created by a half run init()/cealnup() function > crashes my machine it's a inconvenience but it would probably motivate > me to find and fix the issue quicker :-) > I see, but as much as I would like to have init() to cleanup after itself, that should might not happen. What if the base class provided macro TEST_REGISTER does that in case of both success and failures? #define TEST_REGISTER(klass) \ int main(int argc, char *argv[]) \ { \ int ret = klass().execute(); \ cleanup(); \ \ return ret; \ } Thanks j > > > > > > return ret; > > > > > > > > - return run(); > > > > + ret = run(); > > > > + > > > > + cleanup(); > > > > + > > > > + return ret; > > > > } > > > > -- > > Regards, > > > > Laurent Pinchart > > > > > > > > -- > Regards, > Niklas Söderlund
diff --git a/test/test.cpp b/test/test.cpp index 4e7779e750d56687..1bb6ebcb9e8acf18 100644 --- a/test/test.cpp +++ b/test/test.cpp @@ -13,7 +13,6 @@ Test::Test() Test::~Test() { - cleanup(); } int Test::execute() @@ -24,5 +23,9 @@ int Test::execute() if (ret < 0) return ret; - return run(); + ret = run(); + + cleanup(); + + return ret; }
Calling the cleanup() function in the base class Test destructor only calls the base class empty cleanup() function, not the overloaded one. This results in tests not cleaning up after themself. Solve this by explicitly calling the cleanup() function from execute(). This was discovered while running valgrind on tests where objects where allocated in init() and freed in cleanup(). Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- test/test.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)