From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.120]) by sourceware.org (Postfix) with ESMTP id 7913A3858D38 for ; Wed, 12 Aug 2020 21:40:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 7913A3858D38 Received: from mail-vs1-f69.google.com (mail-vs1-f69.google.com [209.85.217.69]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-143-hXIaWItqMGS1y8EjLd-1_g-1; Wed, 12 Aug 2020 17:39:57 -0400 X-MC-Unique: hXIaWItqMGS1y8EjLd-1_g-1 Received: by mail-vs1-f69.google.com with SMTP id n124so851795vsd.16 for ; Wed, 12 Aug 2020 14:39:57 -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; bh=uw6ESIfqUDnpoHvi6dyz59kBw8kfkHerLUcF0BPV61o=; b=d/P7SCITk0iXxqNd3vzfAs6RMih35nzCZNLQZdITFpp3wRnAMel4vjVDLnqXcS5eXz 1nkCQWCOoVbHNAOASVoVT65dcXgcL6cWGDJhnem4yqzjHbuSfMPhtx7+nCe/2DFi07L+ LiAQlRkx50c7sFmnFyarVlWl3PhwsxQiubEAnUkOzMcArGK+Y87ukZv5EQIXRzD9783r Ro9/j1Br4nCRi/kBgF9iSRlDbxrMFC9rPbaNCe5JmwvKKliQv5tcprZKVmXdws0Temsn 3YHZcDvm+YPytkrpf0U4GPWJa8jGv9Ze27HtE03RyLGX9khK3Aa0UZ4MpR0pgcUtJ6DX HqBw== X-Gm-Message-State: AOAM531ViYm1JA0Yb+lXB6dx7/WUKFdXj0d3htJxwt8OFVmYOWQCcYFm xZJsBM7wa1O8KvkAK93Y8BypgFV4ZiwXx/0owLkPSdJiIRvSqQry0cYzntfigK4nSfkzGmY/H9F ej4IOp4yM8AhVlVXjX1xTnFfaKxmygV8ybmfc X-Received: by 2002:ab0:348c:: with SMTP id c12mr1057595uar.89.1597268396445; Wed, 12 Aug 2020 14:39:56 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzzEMHz0k4bpVy1E4+KvfGbJSENPB1PXoB2ynWKGWeKExb230X6JJTiBdmk1wXNzlN6Xq9rmqI8tIeRcVoLL4o= X-Received: by 2002:ab0:348c:: with SMTP id c12mr1057582uar.89.1597268396207; Wed, 12 Aug 2020 14:39:56 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Aaron Merey Date: Wed, 12 Aug 2020 17:39:45 -0400 Message-ID: Subject: Re: [PATCH] debuginfod-support.c: Replace globals with user_data To: Simon Marchi Cc: gdb-patches@sourceware.org X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: multipart/mixed; boundary="0000000000005e2bd805acb507dd" X-Spam-Status: No, score=-10.3 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3, 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: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 12 Aug 2020 21:40:05 -0000 --0000000000005e2bd805acb507dd Content-Type: text/plain; charset="UTF-8" On Sat, Aug 1, 2020 at 10:45 AM Simon Marchi wrote: > > > From 1913317cb95d02c3244e5d91b7b59e33cfe991c5 Mon Sep 17 00:00:00 2001 > > From: Aaron Merey > > Date: Fri, 31 Jul 2020 18:25:48 -0400 > > Subject: [PATCH] debuginfod-support.c: Replace globals with user_data > > > > Store query information in user_data struct instead of global > > variables. Also include DEBUGINFOD_CFLAGS in INTERNAL_CFLAG_BASE. > > The DEBUGINFOD_CFLAGS change looks good, but it's unrelated to the topic of this patch, > please make it its own patch, with its own justification. > > > diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c > > index f4a227b040..9325bf8651 100644 > > --- a/gdb/debuginfod-support.c > > +++ b/gdb/debuginfod-support.c > > @@ -43,29 +43,39 @@ debuginfod_debuginfo_query (const unsigned char *build_id, > > #else > > #include > > > > -/* TODO: Use debuginfod API extensions instead of these globals. */ > > -static std::string desc; > > -static std::string fname; > > -static bool has_printed; > > +struct user_data > > +{ > > + std::string desc; > > + std::string fname; > > + bool has_printed; > > + > > + user_data (std::string desc, std::string fname, bool has_printed) > > + : desc (desc), fname (fname), has_printed (has_printed) > > + { } > > Put the constructor before the fields. Make the fields const if they are not intended > to be changed once the object has been created. > > Using `std::string` causes unnecessary copying to happen. Just use `const char *` > (`const char * const` for the fields) or gdb::string_view, if you want to be more > generic (but all callers are `const char *`, so it's not really necessary). That's > assuming that the lifetime of those strings will always be longer than the lifetime > of the debuginfo client. I think that's the case, since the debug info client is > always created and destroyed in the same function. > > > +}; > > > > static int > > progressfn (debuginfod_client *c, long cur, long total) > > { > > + struct user_data *data = static_cast (debuginfod_get_user_data (c)); > > This line is too long, just skip the `struct` keyword and you'll be fine. Also, space > before `*`. > > > + > > if (check_quit_flag ()) > > { > > printf_filtered ("Cancelling download of %s %ps...\n", > > - desc.c_str (), > > - styled_string (file_name_style.style (), fname.c_str ())); > > + data->desc.c_str (), > > + styled_string (file_name_style.style (), > > + data->fname.c_str ())); > > return 1; > > } > > > > - if (!has_printed && total != 0) > > + if (! data->has_printed && total != 0) > > Remove space after `!`. Hi Simon, Thanks for the review, I've updated the patch and moved the Makefile.in change to a separate patch. Aaron --0000000000005e2bd805acb507dd Content-Type: text/x-patch; charset="US-ASCII"; name="0001-debuginfod-support.c-Replace-globals-with-user_data.patch" Content-Disposition: attachment; filename="0001-debuginfod-support.c-Replace-globals-with-user_data.patch" Content-Transfer-Encoding: base64 Content-ID: X-Attachment-Id: f_kdrw9oyj0 RnJvbSA5MjIxNzkwNTgyMThhODJmOWI3NGUzMjgyNDllNjcyNDRhZGFlM2ZjIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBBYXJvbiBNZXJleSA8YW1lcmV5QHJlZGhhdC5jb20+CkRhdGU6 IFdlZCwgMTIgQXVnIDIwMjAgMTc6MjM6NDIgLTA0MDAKU3ViamVjdDogW1BBVENIIDIvMl0gZGVi dWdpbmZvZC1zdXBwb3J0LmM6IFJlcGxhY2UgZ2xvYmFscyB3aXRoIHVzZXJfZGF0YQoKU3RvcmUg cXVlcnkgaW5mb3JtYXRpb24gaW4gdXNlcl9kYXRhIHN0cnVjdCBpbnN0ZWFkIG9mIGdsb2JhbCB2 YXJpYWJsZXMuCgpnZGIvQ2hhbmdlTG9nOgoKCSogZGVidWdpbmZvZC1zdXBwb3J0LmM6IFJlcGxh Y2UgZ2xvYmFsIHZhcmlhYmxlcyB3aXRoIHVzZXJfZGF0YS4KLS0tCiBnZGIvQ2hhbmdlTG9nICAg ICAgICAgICAgfCAgNCArKysrCiBnZGIvZGVidWdpbmZvZC1zdXBwb3J0LmMgfCAzOCArKysrKysr KysrKysrKysrKysrKysrLS0tLS0tLS0tLS0tLS0tLQogMiBmaWxlcyBjaGFuZ2VkLCAyNiBpbnNl cnRpb25zKCspLCAxNiBkZWxldGlvbnMoLSkKCmRpZmYgLS1naXQgYS9nZGIvQ2hhbmdlTG9nIGIv Z2RiL0NoYW5nZUxvZwppbmRleCAwZTAzNWUyMzNmLi5hZDU0YzQ5ODY0IDEwMDY0NAotLS0gYS9n ZGIvQ2hhbmdlTG9nCisrKyBiL2dkYi9DaGFuZ2VMb2cKQEAgLTEsMyArMSw3IEBACisyMDIwLTA4 LTEyICBBYXJvbiBNZXJleSAgPGFtZXJleUByZWRoYXQuY29tPgorCisJKiBkZWJ1Z2luZm9kLXN1 cHBvcnQuYzogUmVwbGFjZSBnbG9iYWwgdmFyaWFibGVzIHdpdGggdXNlcl9kYXRhLgorCiAyMDIw LTA4LTEyICBBYXJvbiBNZXJleSAgPGFtZXJleUByZWRoYXQuY29tPgogCiAJKiBNYWtlZmlsZS5p biAoREVCVUdJTkZPRF9DRkxBR1MsIERFQlVHSU5GT0RfTElCUyk6IE5ldyB2YXJpYWJsZXMuCmRp ZmYgLS1naXQgYS9nZGIvZGVidWdpbmZvZC1zdXBwb3J0LmMgYi9nZGIvZGVidWdpbmZvZC1zdXBw b3J0LmMKaW5kZXggZjRhMjI3YjA0MC4uM2Y1MWFhYWY0MyAxMDA2NDQKLS0tIGEvZ2RiL2RlYnVn aW5mb2Qtc3VwcG9ydC5jCisrKyBiL2dkYi9kZWJ1Z2luZm9kLXN1cHBvcnQuYwpAQCAtNDMsMjkg KzQzLDM3IEBAIGRlYnVnaW5mb2RfZGVidWdpbmZvX3F1ZXJ5IChjb25zdCB1bnNpZ25lZCBjaGFy ICpidWlsZF9pZCwKICNlbHNlCiAjaW5jbHVkZSA8ZWxmdXRpbHMvZGVidWdpbmZvZC5oPgogCi0v KiBUT0RPOiBVc2UgZGVidWdpbmZvZCBBUEkgZXh0ZW5zaW9ucyBpbnN0ZWFkIG9mIHRoZXNlIGds b2JhbHMuICAqLwotc3RhdGljIHN0ZDo6c3RyaW5nIGRlc2M7Ci1zdGF0aWMgc3RkOjpzdHJpbmcg Zm5hbWU7Ci1zdGF0aWMgYm9vbCBoYXNfcHJpbnRlZDsKK3N0cnVjdCB1c2VyX2RhdGEKK3sKKyAg dXNlcl9kYXRhIChjb25zdCBjaGFyICpkZXNjLCBjb25zdCBjaGFyICpmbmFtZSwgYm9vbCBoYXNf cHJpbnRlZCkKKyAgICA6IGRlc2MgKGRlc2MpLCBmbmFtZSAoZm5hbWUpLCBoYXNfcHJpbnRlZCAo aGFzX3ByaW50ZWQpCisgIHsgfQorCisgIGNvbnN0IGNoYXIgKiBjb25zdCBkZXNjOworICBjb25z dCBjaGFyICogY29uc3QgZm5hbWU7CisgIGJvb2wgaGFzX3ByaW50ZWQ7Cit9OwogCiBzdGF0aWMg aW50CiBwcm9ncmVzc2ZuIChkZWJ1Z2luZm9kX2NsaWVudCAqYywgbG9uZyBjdXIsIGxvbmcgdG90 YWwpCiB7CisgIHVzZXJfZGF0YSAqZGF0YSA9IHN0YXRpY19jYXN0PHVzZXJfZGF0YSAqPiAoZGVi dWdpbmZvZF9nZXRfdXNlcl9kYXRhIChjKSk7CisKICAgaWYgKGNoZWNrX3F1aXRfZmxhZyAoKSkK ICAgICB7CiAgICAgICBwcmludGZfZmlsdGVyZWQgKCJDYW5jZWxsaW5nIGRvd25sb2FkIG9mICVz ICVwcy4uLlxuIiwKLQkJICAgICAgIGRlc2MuY19zdHIgKCksCi0JCSAgICAgICBzdHlsZWRfc3Ry aW5nIChmaWxlX25hbWVfc3R5bGUuc3R5bGUgKCksIGZuYW1lLmNfc3RyICgpKSk7CisJCSAgICAg ICBkYXRhLT5kZXNjLAorCQkgICAgICAgc3R5bGVkX3N0cmluZyAoZmlsZV9uYW1lX3N0eWxlLnN0 eWxlICgpLCBkYXRhLT5mbmFtZSkpOwogICAgICAgcmV0dXJuIDE7CiAgICAgfQogCi0gIGlmICgh aGFzX3ByaW50ZWQgJiYgdG90YWwgIT0gMCkKKyAgaWYgKCFkYXRhLT5oYXNfcHJpbnRlZCAmJiB0 b3RhbCAhPSAwKQogICAgIHsKICAgICAgIC8qIFByaW50IHRoaXMgbWVzc2FnZSBvbmx5IG9uY2Uu ICAqLwotICAgICAgaGFzX3ByaW50ZWQgPSB0cnVlOworICAgICAgZGF0YS0+aGFzX3ByaW50ZWQg PSB0cnVlOwogICAgICAgcHJpbnRmX2ZpbHRlcmVkICgiRG93bmxvYWRpbmcgJXMgJXBzLi4uXG4i LAotCQkgICAgICAgZGVzYy5jX3N0ciAoKSwKLQkJICAgICAgIHN0eWxlZF9zdHJpbmcgKGZpbGVf bmFtZV9zdHlsZS5zdHlsZSAoKSwgZm5hbWUuY19zdHIgKCkpKTsKKwkJICAgICAgIGRhdGEtPmRl c2MsCisJCSAgICAgICBzdHlsZWRfc3RyaW5nIChmaWxlX25hbWVfc3R5bGUuc3R5bGUgKCksIGRh dGEtPmZuYW1lKSk7CiAgICAgfQogCiAgIHJldHVybiAwOwpAQCAtOTgsMTAgKzEwNiw5IEBAIGRl YnVnaW5mb2Rfc291cmNlX3F1ZXJ5IChjb25zdCB1bnNpZ25lZCBjaGFyICpidWlsZF9pZCwKICAg aWYgKGMgPT0gbnVsbHB0cikKICAgICByZXR1cm4gc2NvcGVkX2ZkICgtRU5PTUVNKTsKIAotICBk ZXNjID0gc3RkOjpzdHJpbmcgKCJzb3VyY2UgZmlsZSIpOwotICBmbmFtZSA9IHN0ZDo6c3RyaW5n IChzcmNwYXRoKTsKLSAgaGFzX3ByaW50ZWQgPSBmYWxzZTsKKyAgc3RydWN0IHVzZXJfZGF0YSBk YXRhICgic291cmNlIGZpbGUiLCBzcmNwYXRoLCBmYWxzZSk7CiAKKyAgZGVidWdpbmZvZF9zZXRf dXNlcl9kYXRhIChjLCAmZGF0YSk7CiAgIHNjb3BlZF9mZCBmZCAoZGVidWdpbmZvZF9maW5kX3Nv dXJjZSAoYywKIAkJCQkJYnVpbGRfaWQsCiAJCQkJCWJ1aWxkX2lkX2xlbiwKQEAgLTEzNiwxMSAr MTQzLDEwIEBAIGRlYnVnaW5mb2RfZGVidWdpbmZvX3F1ZXJ5IChjb25zdCB1bnNpZ25lZCBjaGFy ICpidWlsZF9pZCwKICAgaWYgKGMgPT0gbnVsbHB0cikKICAgICByZXR1cm4gc2NvcGVkX2ZkICgt RU5PTUVNKTsKIAotICBkZXNjID0gc3RkOjpzdHJpbmcgKCJzZXBhcmF0ZSBkZWJ1ZyBpbmZvIGZv ciIpOwotICBmbmFtZSA9IHN0ZDo6c3RyaW5nIChmaWxlbmFtZSk7Ci0gIGhhc19wcmludGVkID0g ZmFsc2U7CiAgIGNoYXIgKmRuYW1lID0gbnVsbHB0cjsKKyAgc3RydWN0IHVzZXJfZGF0YSBkYXRh ICgic2VwYXJhdGUgZGVidWcgaW5mbyBmb3IiLCBmaWxlbmFtZSwgZmFsc2UpOwogCisgIGRlYnVn aW5mb2Rfc2V0X3VzZXJfZGF0YSAoYywgJmRhdGEpOwogICBzY29wZWRfZmQgZmQgKGRlYnVnaW5m b2RfZmluZF9kZWJ1Z2luZm8gKGMsIGJ1aWxkX2lkLCBidWlsZF9pZF9sZW4sICZkbmFtZSkpOwog CiAgIGlmIChmZC5nZXQgKCkgPCAwICYmIGZkLmdldCAoKSAhPSAtRU5PRU5UKQotLSAKMi4yNS40 Cgo= --0000000000005e2bd805acb507dd--