On Thu, 12 Aug 2021 12:55:20 +0300, Andy Shevchenko wrote: > On Wed, Aug 11, 2021 at 10:28:25PM +0200, Heiner Kallweit wrote: > > On 11.08.2021 17:45, Andy Shevchenko wrote: > > > On Fri, Aug 06, 2021 at 11:15:15PM +0200, Heiner Kallweit wrote: > > >> Replace the ugly cast of the return_value pointer with proper usage. > > >> In addition use dmi_match() instead of open-coding it. > > > > > > ... > > > > > >> - acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, > > >> - (void **)&found); > > >> + acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &err); > > >> > > >> - return found; > > >> + return !IS_ERR(err); > > > > > > Shouldn't you also check the status of acpi_get_device()? > > > > This shouldn't be needed because err isn't touched if function fails. > > For the sake of clearness of the code I would do it. This brings us back to how awkward the API is. Most callers don't bother checking the return value of acpi_get_devices() because it's useless in practice. But I agree that in theory it could return with an error and then it would be nicer to catch that. > (...) But in any case what > really hurt my eye is the last line here. To me sounds like > > if (IS_ERR(err)) > return false; > return true; > > is much better to read (and I bet the compiler will generate the very same > code for it). Somehow the assembly code differs, but I'm unable to see the relation between your proposed change and the assembly code changes. That's why I hate modern compilers. They pretend to be smart, but what they are essentially is unstable, and this ruins any attempt at such trivial comparisons. Sad. Personally I don't really care, Heiner's code did not strike me as being hard to read in the first place. I tend to avoid conditionals when possible. -- Jean Delvare SUSE L3 Support
