From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-il1-x143.google.com (mail-il1-x143.google.com [IPv6:2607:f8b0:4864:20::143]) by sourceware.org (Postfix) with ESMTPS id 05919385DC3E for ; Tue, 7 Apr 2020 21:32:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 05919385DC3E Received: by mail-il1-x143.google.com with SMTP id p13so4757786ilp.3 for ; Tue, 07 Apr 2020 14:32:53 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=Hj2KLILPubXaNv6xvS2nZqBDNngXhsw6WB9jpk8Hinc=; b=I6KsOxvJt9ibpwy16sptB/GCCGyHrcAmC5GWsBVG/k2Z1egkkIdacsZiVCHVYNtm3m Zjizbp3reQhr9lvYngEE+/WoDdG4tAgdI913CQldtEVrsHPn1uXhtH7Mc7E/cz2RKLVK AUXcH9SYC/RSEC8BfEWW0uTig+/CwOioUwcw7gRMAyBz+bXfCOkaOY/6IwLTWbXZpKr2 tT2pgEMS5Y8NpCAegteFwiAVE6Hm7SE6j9Csy11CBk8y8+oH5F3iGjtCncj7kpp1VT5c m62bQiJDWv2wIjV2GuQ3d6oxm0d7JP6SomKnRpdfqru017ju0Ul7tEbQo8r4EAW3L7rk yPkQ== X-Gm-Message-State: AGi0Pubd5FGXqS7qsMU49IFTDoEHKXK4+HtEWOpxx0FVCV9v3w4TpafD 1kyD8dNhNDk1R5gkGJWghGvZDJ3mA1Rhwz6ZeCk3PA== X-Google-Smtp-Source: APiQypJs3jqa1FT/v5JLOgy79u2j2fWjMa7xmwO5EXV42tfqw/qcks8Gta3Lhcj+JOyj8hVKxfmw+OwepcZejQh+l5g= X-Received: by 2002:a92:de03:: with SMTP id x3mr4918356ilm.242.1586295172363; Tue, 07 Apr 2020 14:32:52 -0700 (PDT) MIME-Version: 1.0 References: <20200403215356.186742-1-gprocida@google.com> <87eeszpfus.fsf@seketeli.org> In-Reply-To: <87eeszpfus.fsf@seketeli.org> From: Giuliano Procida Date: Tue, 7 Apr 2020 22:32:36 +0100 Message-ID: Subject: Re: [PATCH 1/2] abidiff: More compact references to prior diffs. To: Dodji Seketeli Cc: libabigail@sourceware.org, kernel-team@android.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-39.0 required=5.0 tests=BAYES_00, DKIMWL_WL_MED, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, ENV_AND_HDR_SPF_MATCH, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, USER_IN_DEF_DKIM_WL, USER_IN_DEF_SPF_WL 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: libabigail@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libabigail mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 07 Apr 2020 21:32:54 -0000 All done. v2 patches on the way. Giuliano. On Tue, 7 Apr 2020 at 14:47, Dodji Seketeli wrote: > > Hello, > > I like the patch overall, I just have some nits to pick I guess :-) > > Giuliano Procida a =C3=A9crit: > > [...] > > > diff --git a/src/abg-reporter-priv.cc b/src/abg-reporter-priv.cc > > index 883c815e..323de503 100644 > > --- a/src/abg-reporter-priv.cc > > +++ b/src/abg-reporter-priv.cc > > @@ -507,11 +507,11 @@ represent(const var_diff_sptr &diff, > > > > if (d->currently_reporting()) > > { > > - out << " as being reported\n"; > > + out << ", as being reported\n"; > > } > > In general, throughout the code we try to only use braces when > necessary. I know this change didn't create this situation here (I must > have missed it from a previous patch or something) but could you please > remove the brace here, for the sake of consistency with the rest of the > code. > > > else if (d->reported_once()) > > { > > - out << " as reported earlier\n"; > > + out << ", as reported earlier\n"; > > } > > Likewise. > > [...] > > > @@ -527,13 +527,20 @@ represent(const var_diff_sptr &diff, > > if (ctxt->get_reporter()->diff_to_be_reported(d.get())) > > { > > out << indent > > - << "type of '" << pretty_representation << "' changed:\= n"; > > + << "type of '" << pretty_representation << "' changed"; > > if (d->currently_reporting()) > > - out << indent << " details are being reported\n"; > > + { > > + out << ", as being reported\n"; > > + } > > Likewise. > > else if (d->reported_once()) > > - out << indent << " details were reported earlier\n"; > > + { > > + out << ", as reported earlier\n"; > > + } > > Likewise. > > else > > - d->report(out, indent + " "); > > + { > > + out << ":\n"; > > + d->report(out, indent + " "); > > + } > > Likewise. > > [...] > > > Matthias Maennich a =C3=A9crit: > > [...] > > >>+++ b/src/abg-reporter-priv.cc > >>@@ -507,11 +507,11 @@ represent(const var_diff_sptr &diff, > >> > >> if (d->currently_reporting()) > >> { > >>- out << " as being reported\n"; > >>+ out << ", as being reported\n"; > > > > No need for the braces. > > Agreed. > > Thank you Matthias for the review. > > Cheers, > > -- > Dodji