On Wed, Jun 24, 2020 at 3:37 PM Robin Murphy robin.murphy@arm.com wrote:
On 2020-06-24 12:41, Andrzej Hajda wrote:
Many resource acquisition functions return error value encapsulated in pointer instead of integer value. To simplify coding we can use macro which will accept both types of error. With this patch user can use: probe_err(dev, ptr, ...) instead of: probe_err(dev, PTR_ERR(ptr), ...) Without loosing old functionality: probe_err(dev, err, ...)
Personally I'm not convinced that simplification has much value, and I'd say it *does* have a significant downside. This:
if (IS_ERR(x)) do_something_with(PTR_ERR(x));is a familiar and expected pattern when reading/reviewing code, and at a glance is almost certainly doing the right thing. If I see this, on the other hand:
if (IS_ERR(x)) do_something_with(x);
I don't consider your arguments strong enough. You are appealing to one pattern vs. new coming *pattern* just with a different name and actually much less characters to parse. We have a lot of clean ups like this, have you protested against them? JFYI: they are now *established patterns* and saved a ton of LOCs in some of which even were typos.
my immediate instinct is to be suspicious, and now I've got to go off and double-check that if do_something_with() really expects a pointer it's also robust against PTR_ERR values. Off-hand I can't think of any APIs that work that way in the areas with which I'm familiar, so it would be a pretty unusual and non-obvious thing.