From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (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 59D952094606F for ; Fri, 4 May 2018 09:20:05 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 16DD4406C74A; Fri, 4 May 2018 16:20:05 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-129.rdu2.redhat.com [10.10.120.129]) by smtp.corp.redhat.com (Postfix) with ESMTP id 2A7142166BAE; Fri, 4 May 2018 16:20:03 +0000 (UTC) To: Jaben Carsey , edk2-devel@lists.01.org Cc: Liming Gao , Yonghong Zhu References: <8147aa0e203f749165bbcb5da3b4f61d81d9924f.1525449622.git.jaben.carsey@intel.com> From: Laszlo Ersek Message-ID: Date: Fri, 4 May 2018 18:20:03 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <8147aa0e203f749165bbcb5da3b4f61d81d9924f.1525449622.git.jaben.carsey@intel.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Fri, 04 May 2018 16:20:05 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Fri, 04 May 2018 16:20:05 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH v1 1/1] BaseTools: Ecc - add dict for config file to internal translation X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 04 May 2018 16:20:06 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 05/04/18 18:01, Jaben Carsey wrote: > Cc: Liming Gao > Cc: Yonghong Zhu > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jaben Carsey > --- > BaseTools/Source/Python/Ecc/Configuration.py | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/BaseTools/Source/Python/Ecc/Configuration.py b/BaseTools/Source/Python/Ecc/Configuration.py > index b5b583be8c4a..8bc6975c3b41 100644 > --- a/BaseTools/Source/Python/Ecc/Configuration.py > +++ b/BaseTools/Source/Python/Ecc/Configuration.py > @@ -1,7 +1,7 @@ > ## @file > # This file is used to define class Configuration > # > -# Copyright (c) 2008 - 2017, Intel Corporation. All rights reserved.
> +# Copyright (c) 2008 - 2018, Intel Corporation. All rights reserved.
> # This program and the accompanying materials > # are licensed and made available under the terms and conditions of the BSD License > # which accompanies this distribution. The full text of the license may be found at > @@ -20,6 +20,10 @@ from Common.DataType import * > from Common.String import * > from Common.LongFilePathSupport import OpenLongFilePath as open > > +_ConfigFileToInternalTranslation = { > + "ModifierList":"ModifierSet" > + } > + > ## Configuration > # > # This class is used to define all items in configuration file > @@ -297,6 +301,8 @@ class Configuration(object): > Line = CleanString(Line) > if Line != '': > List = GetSplitValueList(Line, TAB_EQUAL_SPLIT) > + if List[0] in _ConfigFileToInternalTranslation: > + List[0] = _ConfigFileToInternalTranslation[List[0]] > if List[0] not in self.__dict__: > ErrorMsg = "Invalid configuration option '%s' was found" % List[0] > EdkLogger.error("Ecc", EdkLogger.ECC_ERROR, ErrorMsg, File = Filepath, Line = LineNo) > I have some comments: (1) Can you please give a short summary in the commit message why this patch is needed? I suggest we also reference the commit that introduced the problem: eece4292acc80. (2) I think at this point we have only a handful of keywords / config options that we recognize. Is that right? Because, I'd like to suggest the following: - add them all to the dictionary (and they all will be identity-mapped except for "ModifierList":"ModifierSet"), - always go through the dict; don't make it optional. Reasons: - if someone introduces a new option, they won't be able to parse it without adding it first to the dict as well (with identity mapping), - and keeping everything in the dict all the time is good because if someone renames something else next time, their text seach will immediately turn up the mapping as well, and they won't forget to update it right off the bat. It's easier to update an occurrence that your search finds than remembering to add a new translation. (3) In connection with (2), I'm not really a fan of translating List[0] in-place. I think we should have a separate variable for original and translated name. ... In fact, I think this patch is technically incorrect; it translates ModifierList to ModifierSet in-place, which might be good for accessing the renamed object member; however, the option check still -- correctly -- uses the public option name: if List[0] == 'ModifierList': List[1] = GetSplitValueList(List[1], TAB_COMMA_SPLIT) and now it wouldn't match. So even if we do translate List[0] in-place, we'll have to preserve the original option name that was read from the file, for option matching. Thanks Laszlo