From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 3EEB2C400 for ; Sun, 2 Aug 2015 22:58:16 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga102.jf.intel.com with ESMTP; 02 Aug 2015 13:58:15 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,596,1432623600"; d="scan'208";a="760420596" Received: from orsmsx107.amr.corp.intel.com ([10.22.240.5]) by fmsmga001.fm.intel.com with ESMTP; 02 Aug 2015 13:58:15 -0700 Received: from orsmsx158.amr.corp.intel.com (10.22.240.20) by ORSMSX107.amr.corp.intel.com (10.22.240.5) with Microsoft SMTP Server (TLS) id 14.3.224.2; Sun, 2 Aug 2015 13:58:14 -0700 Received: from fmsmsx156.amr.corp.intel.com (10.18.116.74) by ORSMSX158.amr.corp.intel.com (10.22.240.20) with Microsoft SMTP Server (TLS) id 14.3.224.2; Sun, 2 Aug 2015 13:58:14 -0700 Received: from fmsmsx113.amr.corp.intel.com ([169.254.13.127]) by fmsmsx156.amr.corp.intel.com ([169.254.13.124]) with mapi id 14.03.0224.002; Sun, 2 Aug 2015 13:58:14 -0700 From: "Wiles, Keith" To: Thomas Monjalon Thread-Topic: [dpdk-dev] [PATCH] log:Change magic number on RTE_LOG_LEVEL to a define Thread-Index: AQHQzUcSuzbppxLeJkKsUYywZo1ZKJ35NKGAgABuQ4D//6/3AA== Date: Sun, 2 Aug 2015 20:58:12 +0000 Message-ID: References: <1433635446-78275-1-git-send-email-keith.wiles@intel.com> <6687423.9WN7KrYfak@xps13> <4594355.leWO83I6KL@xps13> In-Reply-To: <4594355.leWO83I6KL@xps13> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.254.180.166] Content-Type: text/plain; charset="Windows-1252" Content-ID: <8A480DC70593AA4592451002A6C0EA6E@intel.com> Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH] log:Change magic number on RTE_LOG_LEVEL to a define X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 02 Aug 2015 20:58:16 -0000 On 8/2/15, 3:44 PM, "Thomas Monjalon" wrote: >2015-08-02 19:10, Wiles, Keith: >> On 8/2/15, 12:15 PM, "Thomas Monjalon" >>wrote: >> >2015-06-06 19:04, Keith Wiles: >> >> +# Log level use: RTE_LOG_XXX >> >> +# XXX =3D NOOP, EMERG, ALERT, CRIT, ERR, WARNING, NOTICE, INFO or >>DEBUG >> >> +# Look in rte_log.h for others if any. >> >> +# >> > >> >I think this comment is useless. >>=20 >> I do not think the comment is useless as some may not understand what >> values the Log level can be set too in the future. Not commenting the >> change would be a problem IMO. This is also why the line was moved. > >It is already documented in the API doc. >I agree having some comments in the config files would be convenient but: > - this one is 3 lines long > - currently comments are only used to separate sections >Maybe you can do a oneline: > # Minimum log level compiled: DEBUG, INFO, NOTICE, WARNING or ERR >I think it is important to tell it is a minimum log level, i.e. compiled >logs. >And probably it is not needed to suggest a minimum level higher than ERR. I will reduce the comment to one line, should I move the LOG_HISTORY down to under LOG_LEVEL? > >> >> +enum { >> >> + RTE_LOG_NOOP =3D 0, /**< Noop not used (zero entry) */ >> > >> >NOOP is useless: EMERG may be =3D 1 >>=20 >> Does it really matter if I used RTE_LOG_NOOP, just to make sure someone >> did not try to use zero here. Instead of setting the RTE_LOG_EMERG=3D1, = I >> can change it to be the way you suggest, but I think it does not hurt >> anything does it? > >We avoid adding code without real use which could cause confusion. I will remove NOOP and set EMERG=3D1. > >> >> + RTE_LOG_EMERG, /**< System is unusable. */ >> >> + RTE_LOG_ALERT, /**< Action must be taken immediately. */ >> >> + RTE_LOG_CRIT, /**< Critical conditions. */ >> >> + RTE_LOG_ERR, /**< Error conditions. */ >> >> + RTE_LOG_WARNING, /**< Warning conditions. */ >> >> + RTE_LOG_NOTICE, /**< Normal but significant condition. */ >> >> + RTE_LOG_INFO, /**< Informational. */ >> >> + RTE_LOG_DEBUG /**< Debug-level messages. */ >> >> +}; >> > >> >What is the benefit of this change? >>=20 >> The change is to use a enum in place of using magic numbers, plus you >>get >> the benefit of seeing the enum name in the debugger instead of a number. >> It makes the code more readable IMHO. > >OK so a comment in the commit message could give the debugger >justification. OK will add the debugger comment to the commit log. > > =8B=20 Regards, ++Keith Intel Corporation