From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-x632.google.com (mail-pl1-x632.google.com [IPv6:2607:f8b0:4864:20::632]) by sourceware.org (Postfix) with ESMTPS id 8F5963858D34 for ; Wed, 21 Feb 2024 18:14:08 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 8F5963858D34 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 8F5963858D34 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::632 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708539250; cv=none; b=h+XRTq4EuRwrXAMAtrR12w1JkRsQqg8Xgvjwde06IRBv/fGzeDKa0QdCbsnXrGRo4jBCNxoVjEowsmO2CAtE8KIzlXffCWXGCoumMvTmYEdAGKwXfaYVMAFp519TyXT4evyqYOyHshOGIleuxyT7e9v4hZ/5tj4trcYpwx+eSMY= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1708539250; c=relaxed/simple; bh=RfI57hHDD6LGYz0ve+wuNrkXsAVt6SB7svKfeDH9114=; h=DKIM-Signature:From:To:Subject:Date:Message-Id:MIME-Version; b=Al2kZQFH4aR8p50OOSf9SStK5k+HSZfOGlQyJBei74Eu5V1XLcv5nexyge7hnShzRkZnpByNSugnK0mj3t+Ip9G3QMI8yj8sEahdAKrD82oc5ORmpCYbY6G+uK4ntwnL2qFOTarM9WQ3mN8BaPNh9NDsuQZI/BfPHBReCdCBgQ4= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-pl1-x632.google.com with SMTP id d9443c01a7336-1dbb47852cdso8114095ad.1 for ; Wed, 21 Feb 2024 10:14:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1708539247; x=1709144047; darn=sourceware.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=iLLLJbLlzolBtU4gUv7yGC4GgA2EJn+nTCuC2Y20urI=; b=Utr2lcO8eqWjPufyzC9KoNZvFJPYc7qKQbaoJK4wjw5ieRBqpdSGCOw7buxuvYPVs6 ynD1UVj75FWTqPn/FuFAbEOTvHssx5BkBXRVE5p2PkfEnPJixB7gzNt8NJ143HPkPnjD 5KXf1ZY9cFIqswqptvxhy8B5FhpwledCmx4cefKobdXYtM4TT+MxZXFgbWpPbsu9reQ4 GnyARK2cf1qcbZeUS2Cn4venVGNQqk+DkRF1fI8tr1LsyIdCQ+G9S/SZB5vztrlMXsHS 18M5x5nrgGW4HzTntOmSCp4fmELGYuHDDNC4GKmCRWo4JuXPrZStwrGz158tGGdG8lpr EU9g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1708539247; x=1709144047; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=iLLLJbLlzolBtU4gUv7yGC4GgA2EJn+nTCuC2Y20urI=; b=Gla8avDY+VUHeRrqFPJpfAuzHd3A0Xfeb8r7zRz8yt8Hk2NprpgoQttaFVJj9tWSSW e3wRUn62vJD0I9igrXvZ1o7cJ7UHbV890SoORVLMEr2jOag3mpveZo/FsjKBj9ktqvc3 KlBkkHap4YrpIo8VD3gqOFpcHE9YBXDT28gRiBgdh5dUQY1iK8r2YKe8MveFqFzGCjcZ aJ2vH4kCHry9lKb3F4dv5KVy3xMUXRvk9hMjfkkjDJEg5AVheE0rt2xbp/XMn4+SeY3v 1xMGK+pyhnNur4Ta53fCYIGnVMaUiplOXMBBuNait32IRFIoIJeQZlvKw5pD2QpgRt7Z quUA== X-Gm-Message-State: AOJu0Yx5RJ814z9e5KRzuPTU8SUyJ7BL6//I6GSKx0Y+OxuSDf5SygWj jquktL9/YUcQ5y+DJdqXKUHiTRyugmuBxjr9vwZkqQby96ZtreKLQA4Lh1V5 X-Google-Smtp-Source: AGHT+IEJIYv58Eg+CLcOBFG/6ZarFhqscYT4ce2DfLb2umxG7Myb+MmuUxRKPfImoMd8Qpge95pP2Q== X-Received: by 2002:a17:902:c40c:b0:1db:5093:5e23 with SMTP id k12-20020a170902c40c00b001db50935e23mr24451334plk.28.1708539247117; Wed, 21 Feb 2024 10:14:07 -0800 (PST) Received: from lima-default.. ([179.113.44.68]) by smtp.gmail.com with ESMTPSA id kq13-20020a170903284d00b001db5c8202a4sm8438723plb.59.2024.02.21.10.14.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 21 Feb 2024 10:14:06 -0800 (PST) From: Matheus Branco Borella To: gdb-patches@sourceware.org Cc: dark.ryu.550@gmail.com, tom@tromey.com Subject: Re: Re: [PATCH v4] Add support for creating new types from the Python API Date: Wed, 21 Feb 2024 15:11:57 -0300 Message-Id: <20240221181156.3144-1-dark.ryu.550@gmail.com> X-Mailer: git-send-email 2.39.2 In-Reply-To: <871q9pe9p4.fsf@tromey.com> References: <871q9pe9p4.fsf@tromey.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-3.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_ENVFROM_END_DIGIT,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Thanks for the review, I've got a few questions and things to add before I submit the v5, if that's okay. Tom Tromey writes: > WDYT about "make_" instead? Yeah, that works. I don't feel particularly strongly about the names that I picked, I'd mostly just picked them to mirror the internal ones because I didn't have a better idea. I'll switch to `make_*_type`. > Making any sort of type without filling in the details is probably a > recipe for crashes. > > Is there a specific situation you needed this for? The main thing I had in mind was creating a void type, but yeah, it makes sense to avoid exposing the generic type creation function without also providing a proper way to fill in the details, so I'll take that one out. > I'm curious whether this one is really needed, because > gdb.Type.pointer() exists. Like I've told Eli, the main intent behind having it originally was that, assuming one knows how to create a properly-sized pointer for the architecture, one could, without having to rely on any of the type lookup functions. That being said, now that I think about it, I don't think there's any case, when avoiding type lookup for pointer types might be necessary, that can't be solved just as well by using `gdb.Type.pointer()`, while also avoiding the footguns associated with `make_pointer_type`. So I'll take it out, too. > It seems like this could all just create a type_allocator directly and > be simpler, like the 'kind' isn't needed. > Is the none case really possible? > It might be better to just throw an exception from the constructor or > during argument validation or something like that. Most of these fall under the same response, so I'll just reply to them all at once. When I was writing this patch, I had the following in mind: 1st - This patch was first written before GDB switched to C++17, so I had no access to std::optional<>. 2nd - I felt like throwing an exception over doing the `->valid()` check explicitly would be less clear about my intent for people reading the code. The design of `type_storage_owner` follows from those, and I don't feel like changing it to use std::option<> or exceptions would be much of an improvement in readability. Would it really be that much of an improvement? > I think the uses of this could probably use TYPE_ALLOC instead. Isn't that only valid for `struct type`? I don't think I follow. Some of the allocations (and I'm pretty sure at least one has to) happen before the call to `init_*_type`.