| Message ID | 20221004021842.6345-1-laurent.pinchart@ideasonboard.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Quoting Laurent Pinchart via libcamera-devel (2022-10-04 03:18:40) > Hello, > > This small patch series stems from my review of "[PATCH 13/14] > libcamera: pipeline: rkisp1: Factorize errors handling path on start()". > While I agreed that the current error handling pattern in > PipelineHandlerRkISP1::start() is fairly bad, I wasn't thrilled by usage > of goto statements to fix it. This is an alternative proposal. > > The Cleaner class introduced in patch 1/2 took multiple forms before > reaching this one. I first experimented with a class that handled a > single action and thus needed to be instantiated once per action. To > achieve the equivalent of the Cleaner::clear() call I had to either add > an 'if (ret)' statement to the lambda functions, or pass the ret > variable to the constructor of the Cleaner. I found ideas for syntactic > sugar in https://github.com/yuri-kilochek/boost.scope_guard, but in the > end, neither option made me very happy. > > Handling multiple deferred cleanup actions with one Cleaner instance > makes the user code (in 2/2) simpler. The "defer" name for the > Cleaner::defer() function was influenced by an interesting C extension > proposal (https://hal.inria.fr/hal-03059076/document). It uses the name > "guard" for the conceptual equivalent of the Cleaner class, but that > didn't convince me. I'm not sure to really like the "Cleaner" name > though, alternative naming proposals are welcome. I'm not sure I like 'Cleaner' either. To me, this looks very much like the scope_exit experimental C++ features: https://en.cppreference.com/w/cpp/experimental/scope_exit I like the ability to add multiple cleanups to a single instance though. https://github.com/SergiusTheBest/ScopeExit is another interesting implementation, but bases the choice for a Success or Failed return using exceptions, so that's not of any use to us. I (almost) like the syntatic sugar that means lambda's are hidden away: https://github.com/SergiusTheBest/ScopeExit/blob/master/include/ScopeExit/ScopeExit.h SCOPE_EXIT{ cout << "hello"; }; // will be called at the scope exit Which could easily be ScopeExit { }; ... but that's creating a separate instance for each declaration which wouldn't be easy to 'clean up' or cancel..., so I think that's not any better. > One point I'm not happy with is the need to call Cleaner::clear() in the > success path. This is error-prone as it could be forgotten. We could > switch to the opposite, with cleanups disabled by default and an > explicit call in the error paths, but that could also be forgotten, > resulting in missing cleanups in some error paths. The result would > possibly be worse, as error paths are less tested. Another option would > be to require explicit calls in both the success and error paths, and > complain loudly in the destructor if none of those functions have been > called. I'm not sure to be convinced it would be the best idea, although Explicitly requiring either Cleaner.success() or Cleaner.fail() with an error if one isn't called is interesting, but I am weary that then someone will end up simply calling the 'wrong one' on a code path ... But maybe that's worrying about 'possible' bugs, rather than actual bugs. > finding good names for the success/error functions may help. Alternative > ideas are welcome too, as well as opinions regarding whether this is > worth it or not. If there is automatic operations at exit, I'd call it AtScopeExit or ScopeExit ... and use something like (At?)ScopeExit e; e.defer([&]{ std::out << "Failed" << std::endl; }); if (fail) return -EINVAL; e.cancel(); return 0; But it's also just 'deferred work' so perhaps DeferredWorker would be a better class name than Cleaner if it is enforced to be called at both success/failure paths. Remembering to cleanup before a successful exit is a pain, but I bet that's easier to catch if it goes wrong than the alternative, as the 'success' path is likely to be more often used... So I actually think this would be 'safer'? Anyway, overall - I like the idea of a cleanup helper... -- Kieran > > Laurent Pinchart (2): > libcamera: utils: Add Cleaner class > pipeline: rkisp1: Use utils::Cleaner to simplify error handling in > start > > include/libcamera/base/utils.h | 13 +++ > src/libcamera/base/utils.cpp | 130 +++++++++++++++++++++++ > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 31 ++---- > 3 files changed, 154 insertions(+), 20 deletions(-) > > -- > Regards, > > Laurent Pinchart >
Hi Kieran, On Tue, Oct 11, 2022 at 03:51:19PM +0100, Kieran Bingham wrote: > Quoting Laurent Pinchart via libcamera-devel (2022-10-04 03:18:40) > > Hello, > > > > This small patch series stems from my review of "[PATCH 13/14] > > libcamera: pipeline: rkisp1: Factorize errors handling path on start()". > > While I agreed that the current error handling pattern in > > PipelineHandlerRkISP1::start() is fairly bad, I wasn't thrilled by usage > > of goto statements to fix it. This is an alternative proposal. > > > > The Cleaner class introduced in patch 1/2 took multiple forms before > > reaching this one. I first experimented with a class that handled a > > single action and thus needed to be instantiated once per action. To > > achieve the equivalent of the Cleaner::clear() call I had to either add > > an 'if (ret)' statement to the lambda functions, or pass the ret > > variable to the constructor of the Cleaner. I found ideas for syntactic > > sugar in https://github.com/yuri-kilochek/boost.scope_guard, but in the > > end, neither option made me very happy. > > > > Handling multiple deferred cleanup actions with one Cleaner instance > > makes the user code (in 2/2) simpler. The "defer" name for the > > Cleaner::defer() function was influenced by an interesting C extension > > proposal (https://hal.inria.fr/hal-03059076/document). It uses the name > > "guard" for the conceptual equivalent of the Cleaner class, but that > > didn't convince me. I'm not sure to really like the "Cleaner" name > > though, alternative naming proposals are welcome. > > I'm not sure I like 'Cleaner' either. To me, this looks very much like > the scope_exit experimental C++ features: > > https://en.cppreference.com/w/cpp/experimental/scope_exit > > I like the ability to add multiple cleanups to a single instance though. Me too. I considered going for something more similar to scope_exit, but it would have required calling the .release() function on each scope guard individually at the end, which isn't very nice. > https://github.com/SergiusTheBest/ScopeExit is another interesting > implementation, but bases the choice for a Success or Failed return > using exceptions, so that's not of any use to us. Same as https://en.cppreference.com/w/cpp/experimental/scope_fail. > I (almost) like the syntatic sugar that means lambda's are hidden away: > > https://github.com/SergiusTheBest/ScopeExit/blob/master/include/ScopeExit/ScopeExit.h > > SCOPE_EXIT{ cout << "hello"; }; // will be called at the scope exit Almost indeed. > Which could easily be ScopeExit { }; ... but that's creating a separate > instance for each declaration which wouldn't be easy to 'clean up' or > cancel..., so I think that's not any better. Exactly :-) > > One point I'm not happy with is the need to call Cleaner::clear() in the > > success path. This is error-prone as it could be forgotten. We could > > switch to the opposite, with cleanups disabled by default and an > > explicit call in the error paths, but that could also be forgotten, > > resulting in missing cleanups in some error paths. The result would > > possibly be worse, as error paths are less tested. Another option would > > be to require explicit calls in both the success and error paths, and > > complain loudly in the destructor if none of those functions have been > > called. I'm not sure to be convinced it would be the best idea, although > > Explicitly requiring either Cleaner.success() or Cleaner.fail() with an > error if one isn't called is interesting, but I am weary that then > someone will end up simply calling the 'wrong one' on a code path ... > But maybe that's worrying about 'possible' bugs, rather than actual > bugs. That's a possible issue too. > > finding good names for the success/error functions may help. Alternative > > ideas are welcome too, as well as opinions regarding whether this is > > worth it or not. > > If there is automatic operations at exit, I'd call it AtScopeExit or > ScopeExit ... The name sounds good. > and use something like > > > (At?)ScopeExit e; > e.defer([&]{ std::out << "Failed" << std::endl; }); I could also add a constructor that takes a functor, so an instance that would have a single action wouldn't require a separate call to defer(). > > if (fail) > return -EINVAL; > > e.cancel(); > > return 0; > > But it's also just 'deferred work' so perhaps DeferredWorker would be a > better class name than Cleaner if it is enforced to be called at both > success/failure paths. > > > Remembering to cleanup before a successful exit is a pain, but I bet > that's easier to catch if it goes wrong than the alternative, as the > 'success' path is likely to be more often used... So I actually think > this would be 'safer'? I think so too. > Anyway, overall - I like the idea of a cleanup helper... > > > Laurent Pinchart (2): > > libcamera: utils: Add Cleaner class > > pipeline: rkisp1: Use utils::Cleaner to simplify error handling in > > start > > > > include/libcamera/base/utils.h | 13 +++ > > src/libcamera/base/utils.cpp | 130 +++++++++++++++++++++++ > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 31 ++---- > > 3 files changed, 154 insertions(+), 20 deletions(-)
Hello, This small patch series stems from my review of "[PATCH 13/14] libcamera: pipeline: rkisp1: Factorize errors handling path on start()". While I agreed that the current error handling pattern in PipelineHandlerRkISP1::start() is fairly bad, I wasn't thrilled by usage of goto statements to fix it. This is an alternative proposal. The Cleaner class introduced in patch 1/2 took multiple forms before reaching this one. I first experimented with a class that handled a single action and thus needed to be instantiated once per action. To achieve the equivalent of the Cleaner::clear() call I had to either add an 'if (ret)' statement to the lambda functions, or pass the ret variable to the constructor of the Cleaner. I found ideas for syntactic sugar in https://github.com/yuri-kilochek/boost.scope_guard, but in the end, neither option made me very happy. Handling multiple deferred cleanup actions with one Cleaner instance makes the user code (in 2/2) simpler. The "defer" name for the Cleaner::defer() function was influenced by an interesting C extension proposal (https://hal.inria.fr/hal-03059076/document). It uses the name "guard" for the conceptual equivalent of the Cleaner class, but that didn't convince me. I'm not sure to really like the "Cleaner" name though, alternative naming proposals are welcome. One point I'm not happy with is the need to call Cleaner::clear() in the success path. This is error-prone as it could be forgotten. We could switch to the opposite, with cleanups disabled by default and an explicit call in the error paths, but that could also be forgotten, resulting in missing cleanups in some error paths. The result would possibly be worse, as error paths are less tested. Another option would be to require explicit calls in both the success and error paths, and complain loudly in the destructor if none of those functions have been called. I'm not sure to be convinced it would be the best idea, although finding good names for the success/error functions may help. Alternative ideas are welcome too, as well as opinions regarding whether this is worth it or not. Laurent Pinchart (2): libcamera: utils: Add Cleaner class pipeline: rkisp1: Use utils::Cleaner to simplify error handling in start include/libcamera/base/utils.h | 13 +++ src/libcamera/base/utils.cpp | 130 +++++++++++++++++++++++ src/libcamera/pipeline/rkisp1/rkisp1.cpp | 31 ++---- 3 files changed, 154 insertions(+), 20 deletions(-)