public inbox for gcc-rust@gcc.gnu.org
 help / color / mirror / Atom feed
From: Mark Wielaard <mark@klomp.org>
To: Philip Herron <philip.herron@embecosm.com>
Cc: Arthur Cohen <cohenarthur.dev@gmail.com>,
	gcc-rust@gcc.gnu.org, Thomas Schwinge <thomas@codesourcery.com>
Subject: Re: byte/char string representation (Was: [PATCH] Fix byte char and byte string lexing code)
Date: Mon, 4 Oct 2021 00:04:12 +0200	[thread overview]
Message-ID: <YVoo3JO+qXw8sHcE@wildebeest.org> (raw)
In-Reply-To: <CAB2u+n31F2RdraZsytXG-Fu9+w3cg2bKrjqsrJ8BqVe8PXJZrQ@mail.gmail.com>

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


      reply	other threads:[~2021-10-03 22:04 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-21 22:54 [PATCH] Fix byte char and byte string lexing code Mark Wielaard
2021-09-22  9:48 ` Thomas Schwinge
2021-09-22 20:37   ` Mark Wielaard
2021-09-23 11:43     ` Philip Herron
2021-09-23 14:10       ` Arthur Cohen
2021-09-23 20:53         ` byte/char string representation (Was: [PATCH] Fix byte char and byte string lexing code) Mark Wielaard
2021-09-24 11:01           ` Philip Herron
2021-09-25 11:53             ` Mark Wielaard
2021-09-30 10:46               ` Philip Herron
2021-10-03 22:04                 ` Mark Wielaard [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YVoo3JO+qXw8sHcE@wildebeest.org \
    --to=mark@klomp.org \
    --cc=cohenarthur.dev@gmail.com \
    --cc=gcc-rust@gcc.gnu.org \
    --cc=philip.herron@embecosm.com \
    --cc=thomas@codesourcery.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).