From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.93; helo=mga11.intel.com; envelope-from=jordan.l.justen@intel.com; receiver=edk2-devel@lists.01.org Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 29E1721A00AE6 for ; Mon, 18 Mar 2019 11:34:22 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Mar 2019 11:34:22 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.58,494,1544515200"; d="scan'208";a="135358023" Received: from jnewman-mobl7.amr.corp.intel.com (HELO localhost) ([10.254.179.252]) by fmsmga007.fm.intel.com with ESMTP; 18 Mar 2019 11:34:22 -0700 MIME-Version: 1.0 In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14E4051CD@SHSMSX104.ccr.corp.intel.com> References: <20190315051749.6564-1-zhichao.gao@intel.com> <20190315051749.6564-2-zhichao.gao@intel.com> <155263054046.23460.7190580861275117521@jljusten-skl> <3CE959C139B4C44DBEA1810E3AA6F9000B7B0177@SHSMSX101.ccr.corp.intel.com> <155289003042.7045.9736868893658475606@jljusten-skl> <4A89E2EF3DFEDB4C8BFDE51014F606A14E4051CD@SHSMSX104.ccr.corp.intel.com> From: Jordan Justen To: "Gao, Liming" , "Gao, Zhichao" , "edk2-devel@lists.01.org" Cc: "Kinney, Michael D" , Bret Barkelew , Michael Turner , "Gao, Liming" Message-ID: <155293406174.10930.18102544164165817771@jljusten-skl> User-Agent: alot/0.8 Date: Mon, 18 Mar 2019 11:34:21 -0700 Subject: Re: [PATCH V2 01/17] MdePkg/DebugLib.h: Add a new api DebugVPrint for DebugLib X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 18 Mar 2019 18:34:23 -0000 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On 2019-03-18 08:09:36, Gao, Liming wrote: > Jordan: > Coding style document CCS_2_1_Draft.pdf 5.1 General Rules, 5.1.1 > Lines shall be 120 columns, or less. Preferably, limit line > lengths to 80 columns or less. So, I don't think the line length > bigger than 80 and less than 120 violates coding style documents. I think this is clarified as an "exception" base: "When this doesn=E2=80=99t leave sufficient space for a good postfix style comment, extend the line to a total of 120 columns." That doesn't apply to this case. I have never seen a case where I thought this exception was useful in EDK II. I think the exception should be removed, because if a true exceptional case ever did arise, it would be so obvious, no one would need to question it. The only case I've seen (not in EDK II) where going beyond 80 columns seemed to make some sense was a table processed by a build tool. In that case the table has so many columns that line went beyond 80 columns. There a good code-readability reasons for this limit. For one, we can be almost certain that everyone will have 80 columns visible in their editor. But, just as importantly, as the line gets too long, it becomes more challenging to track back to the start of the next line. (Think of newspaper columns, and imagine if they ran all the way across the page.) I'm not sure 80 columns is the "perfect" number, but it does seem that most agree it is probably close. > We should update wiki > https://github.com/tianocore/tianocore.github.io/wiki/Code-Style-C > to match the documentation. Limit line length to 120 characters > instead of 80 characters. Since this is an "exception", and not normally helpful, I don't think we should add it do the wiki. It will only add confusion and lead to more code that doesn't follow the preferred limit. -Jordan >=20 > > -----Original Message----- > > From: Justen, Jordan L > > Sent: Monday, March 18, 2019 2:21 PM > > To: Gao, Zhichao ; edk2-devel@lists.01.org > > Cc: Kinney, Michael D ; Bret Barkelew ; Michael Turner > > ; Gao, Liming > > Subject: RE: [edk2] [PATCH V2 01/17] MdePkg/DebugLib.h: Add a new api D= ebugVPrint for DebugLib > >=20 > > On 2019-03-17 17:14:40, Gao, Zhichao wrote: > > > > -----Original Message----- > > > > From: Justen, Jordan L > > > > Sent: Friday, March 15, 2019 2:16 PM > > > > > > > > On 2019-03-14 22:17:33, Zhichao Gao wrote: > > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1395 > > > > > > > > > > Add a new api DebugVPrint prototype definition in the > > > > > DebugLib header file. This api would expose a print > > > > > routine with VaList parameter. > > > > > > > > These lines seem to be fairly short, with the longest be 54 chars. = I guess not a > > > > problem, but by the recommendation says they could be up to 75 in l= ength. > > > > > > > > https://github.com/tianocore/tianocore.github.io/wiki/Commit-Messag= e- > > > > Format > > > > > > > > > > It is hard for someone to control the characters in a line. > >=20 > > I guess that depends on your editor. :) > >=20 > > > Making the text massage readable base on the rules make more sense. > > > It is easier to control the chars number within 75. Line length less > > > than 76 characters is legal. > >=20 > > Yes, I mentioned it is legal, but it is noticably short. I don't think > > it is so short that it is really a problem, so feel free to leave it > > if you prefer. > >=20 > > > > > > > > According to the style guide: > > > > > > > > "Preferably, limit line lengths to 80 columns or less." > > > > > > > > https://github.com/tianocore/tianocore.github.io/wiki/Code-Style-C > > > > > > > > https://github.com/tianocore- > > > > docs/Docs/raw/master/Specifications/CCS_2_1_Draft.pdf > > > > > > > > But, this line uses 92 columns. > > > > > > > > I think there are similar cases in other patches. > > > > > > Right. This sentence I copied from the comment of DebugPrint > > > function which might be designed for a long time. The CCS might not > > > be designed as such style at that time. For this, it's better to > > > change both of the two sentences. > > > By the way, the whole edk2 repo may contain a lot of lines violated > > > this rule. It takes effort to fix them all. > >=20 > > I agree there are many violations of this in edk2. > >=20 > > Usually, it's not too important to go around fixing the issues that > > already happened, but if you are changing or adding some code, then > > you can try to make sure the new lines you add are good. > >=20 > > -Jordan