From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Stefan Kangas Newsgroups: gmane.emacs.bugs Subject: bug#45562: [PATCH] Fix "comparison always the same" warnings found by lgtm Date: Fri, 1 Jan 2021 10:21:24 -0600 Message-ID: References: <83zh1ugj30.fsf@gnu.org> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="29362"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 45562@debbugs.gnu.org To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Fri Jan 01 17:22:11 2021 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1kvNBp-0007W5-Oh for geb-bug-gnu-emacs@m.gmane-mx.org; Fri, 01 Jan 2021 17:22:09 +0100 Original-Received: from localhost ([::1]:52714 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1kvNBo-0002hP-L1 for geb-bug-gnu-emacs@m.gmane-mx.org; Fri, 01 Jan 2021 11:22:08 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:36690) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kvNBi-0002hC-4Y for bug-gnu-emacs@gnu.org; Fri, 01 Jan 2021 11:22:02 -0500 Original-Received: from debbugs.gnu.org ([209.51.188.43]:51288) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1kvNBh-0006WG-Tv for bug-gnu-emacs@gnu.org; Fri, 01 Jan 2021 11:22:01 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1kvNBh-0006i5-PL for bug-gnu-emacs@gnu.org; Fri, 01 Jan 2021 11:22:01 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Stefan Kangas Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Fri, 01 Jan 2021 16:22:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 45562 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 45562-submit@debbugs.gnu.org id=B45562.160951809325755 (code B ref 45562); Fri, 01 Jan 2021 16:22:01 +0000 Original-Received: (at 45562) by debbugs.gnu.org; 1 Jan 2021 16:21:33 +0000 Original-Received: from localhost ([127.0.0.1]:34601 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1kvNBF-0006hL-BZ for submit@debbugs.gnu.org; Fri, 01 Jan 2021 11:21:33 -0500 Original-Received: from mail-pf1-f181.google.com ([209.85.210.181]:44337) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1kvNBD-0006h6-Bf for 45562@debbugs.gnu.org; Fri, 01 Jan 2021 11:21:32 -0500 Original-Received: by mail-pf1-f181.google.com with SMTP id f9so12620572pfc.11 for <45562@debbugs.gnu.org>; Fri, 01 Jan 2021 08:21:31 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:in-reply-to:references:mime-version:date :message-id:subject:to:cc; bh=HZkxsJ82rtwlEqegZQyTiB6ixIWoXpcacH+wl0HC2zA=; b=uDOq09y59G1Z0s9e+gxiz3Z8EZjDIyGT0xhks4pPhv7e6a2GeIHF9QPCB+sCOe+KmE M4KgNQiMgviNmdB4P0wgeyI5zgpGKeFep4E829DGgDxIk5EFsKa13JBM6C+6A4KZlYOx 6Rhr4o9VXiQNjWxXbKCS5j6SnE9mHSxkGkiabS7SSW3E2v3DYEuPScEmeLA5EX9y+s/q cSSAgcJZEOUD8ubkgCS8JkjbXlkBEh+aSpqn8Wrpy1XS/4pq3pescSaVNC1sYOjYTaB8 lCi20Q/5DSyrcVBL67NOnWKuSa+IuP/S8NQcjCayVrOF9G8lkXs1jLQF30jznoadZ/ly W+Pw== X-Gm-Message-State: AOAM531YNG29U9LIHYICCVUvftfBEX0bDIZFJl6ZWYLzWrpUmBll/00a ubtzOvcW3d+dZ7dryg42HF3WFFMHzrN1yNLx9xiuv/7c4So= X-Google-Smtp-Source: ABdhPJx+6nA4xff51fi96vD/27+n74aBS2A7HxLMYpWtCNYQBBPrxgZ7/V35CJc55QJ73AyOWawhRA2hfpt1RqGpMXI= X-Received: by 2002:a63:e108:: with SMTP id z8mr57546140pgh.363.1609518085519; Fri, 01 Jan 2021 08:21:25 -0800 (PST) Original-Received: from 753933720722 named unknown by gmailapi.google.com with HTTPREST; Fri, 1 Jan 2021 10:21:24 -0600 In-Reply-To: <83zh1ugj30.fsf@gnu.org> X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.io gmane.emacs.bugs:197137 Archived-At: Eli Zaretskii writes: >> The attached patch fixes some warnings found by lgtm.com. > > Thanks. IME, these tools have quite a low signal-to-noise ratio. In > this case: Thanks for the review! Indeed, this is why I asked for some comments. >> --- a/src/buffer.c >> +++ b/src/buffer.c >> @@ -5238,8 +5238,7 @@ init_buffer_once (void) >> PDUMPER_REMEMBER_SCALAR (buffer_local_flags); >> >> /* Need more room? */ >> - if (idx >= MAX_PER_BUFFER_VARS) >> - emacs_abort (); >> + eassert (idx < MAX_PER_BUFFER_VARS); > > This is wrong, because eassert compiles to nothing in the production > build, so it is only good for situations where continuing without > aborting will do something reasonable, or if it will crash anyway in > the very next source line. In this case, there's no way we can > continue, and the programmer evidently wanted us to abort rather than > continue and let us crash later. Right. But we know the value of both idx and MAX_PER_BUFFER_VARS at compile time. So while I understand your argument, it is arguably a judgment call whether or not it is worth making this check also in production builds. IMHO, the eassert has the (minor) benefit of making the intention clearer. That said, AFAICT we call this function only once per lifetime. So I'm happy to leave this out if you prefer. >> --- a/src/fns.c >> +++ b/src/fns.c >> @@ -3847,8 +3847,6 @@ base64_decode_1 (const char *from, char *to, ptrdiff_t length, >> if (c == '=') >> continue; >> >> - if (v1 < 0) >> - return -1; >> value += v1 - 1; >> >> c = value & 0xff; > > I don't think I see why removing the test and the failure return would > be TRT. What did I miss? Because we have above: do { ... } while (v1 < 0); So unless I am missing something the test is always false and we will never reach the return.