From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-4.0 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI,RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by dcvr.yhbt.net (Postfix) with ESMTP id D390C1F619 for ; Tue, 25 Feb 2020 22:02:42 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729148AbgBYWCm (ORCPT ); Tue, 25 Feb 2020 17:02:42 -0500 Received: from pb-smtp21.pobox.com ([173.228.157.53]:61292 "EHLO pb-smtp21.pobox.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729016AbgBYWCl (ORCPT ); Tue, 25 Feb 2020 17:02:41 -0500 Received: from pb-smtp21.pobox.com (unknown [127.0.0.1]) by pb-smtp21.pobox.com (Postfix) with ESMTP id E845FB1A0A; Tue, 25 Feb 2020 17:02:39 -0500 (EST) (envelope-from junio@pobox.com) DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=pobox.com; h=from:to:cc :subject:references:date:in-reply-to:message-id:mime-version :content-type; s=sasl; bh=pNJyXqaPP0bNiPMFGIhRJB+8Ubk=; b=qxHzKR OyGmRRKNZb9Xrsvzzfxbj3DO37UWAr4u3u96V2U9rjHHwcgv30Fu2RaeNlcsDrUT 1hMeVghLtXoPH8yb/J6bv6Ci8CgtQOCzrfJliwoaUJVXsUzHLW0PcA+Vy3nEQFg8 SHgb9iHz17fneN8yLkmWFYqeANw3TPmjO6BOI= DomainKey-Signature: a=rsa-sha1; c=nofws; d=pobox.com; h=from:to:cc :subject:references:date:in-reply-to:message-id:mime-version :content-type; q=dns; s=sasl; b=v7ep6r7rCo83iywUjWzp5c/qQIrYEVos hOHauTH2a4vs57kHMmtz3XMKTmcSFQYqoKtzTJRiHoIRWMEF4mqhOYq0Rd66nZlO 3GVdoAhXqE8NM5AHh1nQc2Egf7zv9sMKhiN/W0WXsa/inOEuFWebFW5DuVX1Snr2 QMIKON3afDU= Received: from pb-smtp21.sea.icgroup.com (unknown [127.0.0.1]) by pb-smtp21.pobox.com (Postfix) with ESMTP id E036CB1A09; Tue, 25 Feb 2020 17:02:39 -0500 (EST) (envelope-from junio@pobox.com) Received: from pobox.com (unknown [34.76.80.147]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by pb-smtp21.pobox.com (Postfix) with ESMTPSA id 065F6B1A08; Tue, 25 Feb 2020 17:02:36 -0500 (EST) (envelope-from junio@pobox.com) From: Junio C Hamano To: Heba Waly Cc: Heba Waly via GitGitGadget , Git Mailing List Subject: Re: [PATCH v5 2/3] advice: revamp advise API References: Date: Tue, 25 Feb 2020 14:02:34 -0800 In-Reply-To: (Heba Waly's message of "Wed, 26 Feb 2020 10:19:25 +1300") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Pobox-Relay-ID: 887B9D64-581A-11EA-AE25-8D86F504CC47-77302942!pb-smtp21.pobox.com Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Heba Waly writes: > I wasn't very happy about having to keep the list of config keys in > memory, but that was a good enough solution for now. If you force your programmers to specify the advice_type as a small integer, and the setting is stored in the config as string keys, somebody MUST have a table to convert from one to the other. So I am not sure if it is even sensible to feel unhappy about having to have a list in the first place. Are we looking for some kind of miracles ;-)? On the other hand, it does bother my sense of aesthetics a lot it our code forces our programmers to give a small integer to us, only so that we convert that integer to a string and use the string to look up a value in a hashtable, every time the program wants a lookup. Performance-wise, that's not a huge downside. It just rubs my sense of code hygiene the wrong way. Especially when the primary way for our programmers to specify which advice they are talking about is by passing an integer, and if we need to have a table indexed by that integer in the program anyway. We could instead do something like: /* advice.h */ #ifndef _ADVICE_H_ #define _ADVICE_H_ 1 extern const char ADVICE_ADD_EMBEDDED_REPO[]; extern const char ADVICE_AM_WORK_DIR[]; ... #endif /* advice.c */ const char ADVICE_ADD_EMBEDDED_REPO[] = "advice.addEmbeddedRepo"; const char ADVICE_ADD_AM_WORK_DIR[] = "advice.amWorkDir"; ... and the callers can still do advise_if_enabled(ADVICE_NESTED_TAG, _(message_advice_nested_tag), tag, object_ref); with the benefit of compiler catching a silly typo, without having to have any "enum-to-string" table while letting the config API layer do any caching transparently. As these calls will never be placed in a performance critical codepath, that might be more appropriate. I dunno.