From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from outbound.soverin.net (outbound.soverin.net [116.202.65.215]) by sourceware.org (Postfix) with ESMTPS id 950C73858D3C for ; Sun, 3 Oct 2021 22:04:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 950C73858D3C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=klomp.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=klomp.org Received: from smtp.soverin.net (unknown [10.10.3.24]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by outbound.soverin.net (Postfix) with ESMTPS id E0BE3600F2; Sun, 3 Oct 2021 22:04:18 +0000 (UTC) Received: from smtp.soverin.net (smtp.soverin.net [159.69.232.138]) by soverin.net Received: by reform (Postfix, from userid 1000) id EF3672E800CE; Mon, 4 Oct 2021 00:04:12 +0200 (CEST) Date: Mon, 4 Oct 2021 00:04:12 +0200 From: Mark Wielaard To: Philip Herron Cc: Arthur Cohen , gcc-rust@gcc.gnu.org, Thomas Schwinge Subject: Re: byte/char string representation (Was: [PATCH] Fix byte char and byte string lexing code) Message-ID: References: <20210921225430.166550-1-mark@klomp.org> <87k0j9ym7r.fsf@euler.schwinge.homeip.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Spam-Status: No, score=-3.9 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, RCVD_IN_DNSWL_LOW, SPF_HELO_PASS, SPF_PASS, TXREP, WEIRD_PORT autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-rust@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: gcc-rust mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 03 Oct 2021 22:04:22 -0000 Hi Philip, On Thu, Sep 30, 2021 at 11:46:30AM +0100, Philip Herron wrote: > Your patch was 99% of the way there to fix the type > resolution so I finished it off for you: > > https://github.com/Rust-GCC/gccrs/pull/698/files And you added the actual backend code needed! So I'll probably say I was maybe 45% there :) This is really useful. It works for me and will enable writing a couple of real tests for byte strings (like we have for str already, that aren't just lexer/parser checks, but which can actually check the contents of the byte strings. > The missing piece was that References and Array's are a type of covariant > type so that an array type can look like this: [_, capacity], so the > inference variable here is the variant so that we need to make sure it has > its own implicit mapping id. You just needed to create one more mapping to > get that implicit id so that the reference type similarly doesn't get into > a loop of looking up itself. Creating implicit types like this could be > made easier, so we should likely add some helpers for this scenario. Thanks, my knowledge of mappings was a bit fuzzy. So even though we know our own array type is fixed, we still need to attach a unique mapping to match against other types which might not be fixed (yet). There is indeed a lot of extra work you need to do and that you can easily get wrong. Creating an empty mapping is a lot of work and it is easy to get the details right. An new_empty_mapping helper method would be great. Maybe it could even be hidden inside the expression or type constructor. I do have a question about why we also create these mappings for the capacity. The capacity is always a fixed usize number. Not just in this case, but always since it is constant expression in a const context. So any comparison of the length will always be a simple constant usize number check. Is using mappings not overkill in that case? We use a Bexpression for the capacity, created by ConstFold::ConstFoldExpr::fold, shouldn't we check it actually folded correctly? Currently when it cannot we just leave it in an error state till it is too late. e.g pub fn main() { let _a: &[u8; 4 / 0]; _a = b"test"; } Results in: rust1: internal compiler error: in const_size_val_to_string, at rust/rust-gcc.cc:210 0x794e67 Gcc_backend::const_size_val_to_string[abi:cxx11](Bexpression*) /home/mark/src/gccrs/gcc/rust/rust-gcc.cc:210 0x794e67 Gcc_backend::const_size_val_to_string[abi:cxx11](Bexpression*) /home/mark/src/gccrs/gcc/rust/rust-gcc.cc:208 0x8c51ef Rust::TyTy::ArrayType::capacity_string[abi:cxx11]() const /home/mark/src/gccrs/gcc/rust/typecheck/rust-tyty.cc:1136 0x8c51ef Rust::TyTy::ArrayType::as_string[abi:cxx11]() const /home/mark/src/gccrs/gcc/rust/typecheck/rust-tyty.cc:1129 0x8d0c8b Rust::TyTy::BaseCoercionRules::visit(Rust::TyTy::ArrayType&) /home/mark/src/gccrs/gcc/rust/typecheck/rust-tyty-coercion.h:166 [...] So it seems we really should check for is_error_expr earlier. And maybe just get the integer value and store/compare that instead of the Bexpression? P.S. Don't let this stop you from committing the code as is, it does work. We can rework it afterwards if the above comments make any sense. Cheers, Mark