From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 8DA013850438 for ; Tue, 22 Jun 2021 23:37:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8DA013850438 Received: from mail-qv1-f69.google.com (mail-qv1-f69.google.com [209.85.219.69]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-117-fvCUHd7jNV-twcTS-Ey_cw-1; Tue, 22 Jun 2021 19:37:04 -0400 X-MC-Unique: fvCUHd7jNV-twcTS-Ey_cw-1 Received: by mail-qv1-f69.google.com with SMTP id p5-20020a0ccb850000b029025849db65e9so811832qvk.23 for ; Tue, 22 Jun 2021 16:37:04 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:message-id:subject:from:to:date:in-reply-to :references:user-agent:mime-version:content-transfer-encoding; bh=j48egKFJMRwHj8G8ZHlvLNZ9kinB9VB6tWYFUv20l5Q=; b=Vn6Tp/32FNMvdPOzQrW52MFiAq5cUJ6U1t8BnbbuVAZbEEcr7i0LBpAu71W45cBnnI 3kOQreDSW5Xr07LR01f5/hoWvyayaKj1HgsS6HtNRIRX+MBWNquSxWpkk1LhSR4SF+3b TkSJ3Vn+N3kl2JHIY1lsRGsYnhqWlG/D9vHwZbMa4d/pF7sIcauJCHVFRsIhHyZfYiJ/ 8C2/jGmB0SoZmWjOu2PjPM1RcWO9bCugOcTW837QwB87JfkmioFwR+CvZyvdQMEqJBVy Us6CsTj7HLnnFYNroJtBgZeCcNL2msEfdq0h7Nq9LGSpUY+f45AgmJTjwhcTARY0Ds+O Yoww== X-Gm-Message-State: AOAM530RjL3BbYCysFshE3azRaA8O39pbuv+NinGtbBHTd9Qmv1eTg8m aSFtSCZVLlLjyQTnwHYY6aybMIpdPp0hN6Fs4DEP/LqAu3qjXXU7Uqbou95LoVacye+MS5dsbGe fMBH0pU5qeon4+wv86g== X-Received: by 2002:a05:622a:1aa4:: with SMTP id s36mr1155849qtc.337.1624404522490; Tue, 22 Jun 2021 16:28:42 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxtkis5+8iZMcJYSlOFN8a1PNmq4fbcHK0SSR6VFHjlg4Av5kLPwWntoPuWeK4HnVk6ADS9jQ== X-Received: by 2002:a05:622a:1aa4:: with SMTP id s36mr1155839qtc.337.1624404522322; Tue, 22 Jun 2021 16:28:42 -0700 (PDT) Received: from t14s.localdomain (c-73-69-212-193.hsd1.nh.comcast.net. [73.69.212.193]) by smtp.gmail.com with ESMTPSA id y10sm2748273qtm.17.2021.06.22.16.28.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 22 Jun 2021 16:28:41 -0700 (PDT) Message-ID: <4514e92d166a007cdfd6b971a69a295a4d8b6891.camel@redhat.com> Subject: Re: [PATCH 1/13] v2 [PATCH 1/13] Add support for per-location warning groups (PR 74765) From: David Malcolm To: Martin Sebor , gcc-patches Date: Tue, 22 Jun 2021 19:28:41 -0400 In-Reply-To: References: <92db3776-af59-fa20-483b-aa67b17d0751@gmail.com> <47d06c821a53f5bd2246f0fca2c9a693bff6b882.camel@redhat.com> <3a5ea83c-0d91-d123-f537-f8f1223df890@gmail.com> User-Agent: Evolution 3.38.4 (3.38.4-1.fc33) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00, BODY_8BITS, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 22 Jun 2021 23:37:10 -0000 On Tue, 2021-06-22 at 19:18 -0400, David Malcolm wrote: > On Fri, 2021-06-04 at 15:41 -0600, Martin Sebor wrote: > > The attached patch introduces the suppress_warning(), > > warning_suppressed(), and copy_no_warning() APIs without making > > use of them in the rest of GCC.  They are in three files: > > > >    diagnostic-spec.{h,c}: Location-centric overloads. > >    warning-control.cc: Tree- and gimple*-centric overloads. > > > > The location-centric overloads are suitable to use from the > > diagnostic > > subsystem.  The rest can be used from the front ends and the middle > > end. > > [...snip...] > > > +/* Return true if warning OPT is suppressed for decl/expression > > EXPR. > > +   By default tests the disposition for any warning.  */ > > + > > +bool > > +warning_suppressed_p (const_tree expr, opt_code opt /* = > > all_warnings */) > > +{ > > +  const nowarn_spec_t *spec = get_nowarn_spec (expr); > > + > > +  if (!spec) > > +    return get_no_warning_bit (expr); > > + > > +  const nowarn_spec_t optspec (opt); > > +  bool dis = *spec & optspec; > > +  gcc_assert (get_no_warning_bit (expr) || !dis); > > +  return dis; > > Looking through the patch, I don't like the use of "dis" for the "is > suppressed" bool, since... > > [...snip...] > > > + > > +/* Enable, or by default disable, a warning for the statement STMT. > > +   The wildcard OPT of -1 controls all warnings.  */ > > ...I find the above comment to be confusingly worded due to the double- > negative. > > If I'm reading your intent correctly, how about this wording: > > /* Change the supression of warnings for statement STMT. >    OPT controls which warnings are affected. >    The wildcard OPT of -1 controls all warnings. >    If SUPP is true (the default), enable the suppression of the > warnings. >    If SUPP is false, disable the suppression of the warnings.  */ > > ...and rename "dis" to "supp". > > Or have I misread the intent of the patch? > > > + > > +void > > +suppress_warning (gimple *stmt, opt_code opt /* = all_warnings */, > > +                 bool dis /* = true */) > > > +{ > > +  if (opt == no_warning) > > +    return; > > + > > +  const key_type_t key = convert_to_key (stmt); > > + > > +  dis = suppress_warning_at (key, opt, dis) || dis; > > +  set_no_warning_bit (stmt, dis); > > +} > > [...snip...] Also, I think I prefer having a separate entrypoints for the "all warnings" case; on reading through the various patches I think that in e.g.: - TREE_NO_WARNING (*expr_p) = 1; + suppress_warning (*expr_p); I prefer: suppress_warnings (*expr_p); (note the plural) since that way we can grep for them, and it seems like better grammar to me. Both entrypoints could be implemented by a static suppress_warning_1 internally if that makes it easier. In that vein, "unsuppress_warning" seems far clearer to me that "suppress_warning (FOO, false)"; IIRC there are very few uses of this non-default arg (I couldn't find any in a quick look through the v2 kit). Does this make sense? Dave