Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754071AbcDVJRt (ORCPT ); Fri, 22 Apr 2016 05:17:49 -0400 Received: from www.linutronix.de ([62.245.132.108]:52458 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751554AbcDVJRq (ORCPT ); Fri, 22 Apr 2016 05:17:46 -0400 Date: Fri, 22 Apr 2016 11:16:03 +0200 (CEST) From: Thomas Gleixner To: "Du, Changbin" cc: Andrew Morton , Jonathan Corbet , "Paul E. McKenney" , "josh@joshtriplett.org" , Steven Rostedt , Mathieu Desnoyers , "jiangshanlai@gmail.com" , John Stultz , Tejun Heo , "borntraeger@de.ibm.com" , "dchinner@redhat.com" , "linux-doc@vger.kernel.org" , LKML Subject: RE: [PATCH 1/7] debugobjects: make fixup functions return bool instead of int In-Reply-To: <0C18FE92A7765D4EB9EE5D38D86A563A05D1B3D7@SHSMSX103.ccr.corp.intel.com> Message-ID: References: <1461312468-14335-1-git-send-email-changbin.du@intel.com> <1461312468-14335-2-git-send-email-changbin.du@intel.com> <0C18FE92A7765D4EB9EE5D38D86A563A05D1B3D7@SHSMSX103.ccr.corp.intel.com> User-Agent: Alpine 2.11 (DEB 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-Linutronix-Spam-Score: -1.0 X-Linutronix-Spam-Level: - X-Linutronix-Spam-Status: No , -1.0 points, 5.0 required, ALL_TRUSTED=-1,SHORTCIRCUIT=-0.0001,URIBL_BLOCKED=0.001 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2073 Lines: 52 On Fri, 22 Apr 2016, Du, Changbin wrote: > > On Fri, 22 Apr 2016, changbin.du@intel.com wrote: > > > A bad thing is that debug_object_fixup use the return value for > > > arithmetic operation. It confused me that what is the reall return > > > > What's bad about that? The fact that it's used for arithmethic operation or > > that it confused you? > > > It confused me because this is not a common usage. I was confused that what > does he fixup function return? A countable value? But doc says return fixed > or not! It says return 0 for not fixed up and 1 for fixed up. The activate fixup is special and it has been written this way to handle the static initialization case. > if (fixup) > fixed = fixup(addr, state); > debug_objects_fixups += fixed; > In common,for int return 0 indicates success, negative for fail, positive > for something countable. So I think it is better follow this rule. Here is > not of countable, it is Boolean. Yes, it's common for most of the code. This code has been deliberately been written differently. I'm not opposed to change that and improve it, but just slapping bool on it does not really make any difference. > So why not this? > if (fixup && fixup(addr, state)) > debug_objects_fixups++; There is no problem with that per se. > > > + bool (*fixup_init)(void *addr, enum debug_obj_state state); > > > + bool (*fixup_activate)(void *addr, enum debug_obj_state state); > > > + bool (*fixup_destroy)(void *addr, enum debug_obj_state state); > > > + bool (*fixup_free)(void *addr, enum debug_obj_state state); > > > + bool (*fixup_assert_init)(void *addr, enum debug_obj_state state); > > > > So this change will introduce a gazillion of compile warnings because the > > callbacks in the various usage sites are still having 'int' return type. > > > No, I modified all the code who use debugojects API. You do that in the later patches. But patches must be compilable and functional on their own. Compiling this one will emit a gazillion of "initialization from incompatible pointer type" warnings. Thanks, tglx