fitzgen opened PR #3506 from isle
to main
:
This pull request introduces initial ISLE integration for the x64 backend. It is blocked on https://github.com/bytecodealliance/rfcs/pull/15 finishing its final comment period and being accepted.
I'm opening this up early since (a) I have a bunch of working code here that already ports a significant amount of the hand-written clif-to-MachInst lowering for x64, and (b) so that folks who are interested in how ISLE will work can get a chance to dig into more details and actual code rather than just slides I've presented at Cranelift meetings.
I'll keep pushing to this branch/PR as I continue porting more hand-written lowering code to ISLE.
Feel free to comment and give feedback or ask questions!
sunfishcode submitted PR review.
sunfishcode submitted PR review.
sunfishcode created PR review comment:
Similarly, I'd find it clearer to have the SSE version after all the GPR versions.
sunfishcode created PR review comment:
Some error implementations have more helpful messages in their debug output than in their
Display
output, so I suggest using{:?}
instead of{}
here.
sunfishcode created PR review comment:
Should this be
imul
instead ofiadd
? And similar below.
sunfishcode created PR review comment:
As a minor style suggestion, I'd find it easier to read if the
i64
and smaller code was before thei128
code. That way, reading this top-to-bottom, I can start with the simple form, figure out what that's doing, and then look at thei128
version and see how it differs in the obvious way.And similar for the patterns below.
sunfishcode created PR review comment:
Is the
with_type
a predicate that determines whether the pattern matches? Could I suggest the namehas_type
for this?
sunfishcode created PR review comment:
It's not obvious to me what this
<ty
syntax means.
cfallin submitted PR review.
cfallin created PR review comment:
This is an "in-arg" to the left-hand-side extractor; the basic idea is that while the arg-positions of left-hand-side patterns are mostly "outputs" of whatever the matcher is doing (the inverse of a function application on the RHS), some matcher implementations will want an input (in addition to the thing they're matching on) to configure how they match. So here I think
simm32_from_value
is a pattern-matcher that is parameterized on a type.(If one were doing this in e.g. Prolog it'd be just another argument, and dataflow could flow either way during unification. Here we have "argument polarity" instead, which one can think of as an approximation of this...)
I did mostly intend this to be a low-level feature that we could use to define other sorts of extractors/matchers...
@fitzgen: I wonder if there's a way to better write this? It looks like the
ty
in-arg eventually gets down toto_simm32
here which just checks ifty
is less than 64 bits, and skips an "is sign extendable" check if so; but it looks like this may actually be unnecessary (I don't fully remember the semantics of simm32 values on 32 vs 64 bit instructions though)? Or could we just split out the 8/16/32 and 64 bit cases (one of the advantages of just having a list of patterns even!)?
cfallin created PR review comment:
Alternately, we propagate arg polarity to the parser and parse in-args as expressions without the
<
, but then that makes this a type-dependent parse (a la C++) which is kind of not great. We could do it though if we require decls to come before uses of terms...
cfallin submitted PR review.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
This unconditionally writes to the source dir. Crates.io dependencies aren't allowed to write to their source dir.
cargo publish
checks that the source dir stays identical. If the isle file is up to date, this check will currently pass, but if in the future the source dirs are made readonly, this will break.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
Maybe write it to
OUT_DIR
too and let ISLE read it from there?
bjorn3 submitted PR review.
bjorn3 created PR review comment:
The pre-regalloc printing now doesn't make sense anymore. After regalloc it will hide invariant violations.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
This is unconditionally using isle, right? This shouldn't be merged without benchmarking first.
cfallin submitted PR review.
cfallin created PR review comment:
I wonder if the checked-in-generated-code approach could be done as follows: (i) provide a shell-script that regenerates all relevant source; (ii) in CI, fail if generated source is not up-to-date; (iii) do nothing in build.rs, to avoid surprising side-effects in the source tree.
This seems to be the simplest and fastest option, and keeps to the general principle that the source tree is human-managed and not edited by tools except by explicit invocation of a command. It does however require a manual
./update-isle.sh && cargo build
when iterating on the rules. Maybe a way to avoid a footgun here is to error inbuild.rs
if the generated source is out of data. We could do that without puttingislec
in the build deps by recording a hash of the ISLE source alongside generated code, and checking against that hash in build.rs. Thoughts?
cfallin edited PR review comment.
fitzgen submitted PR review.
fitzgen created PR review comment:
Is the
with_type
a predicate that determines whether the pattern matches?Yep
Could I suggest the name
has_type
for this?Nice, its even a character shorter
fitzgen submitted PR review.
sunfishcode submitted PR review.
fitzgen created PR review comment:
Or could we just split out the 8/16/32 and 64 bit cases (one of the advantages of just having a list of patterns even!)?
What's nice about having polymorphic rules instead of a separate rule for each of
i{8,16,32,64}
is that they actually end up as a single branch in the trie/generated code, instead of four different branches.I wonder if there's a way to better write this? It looks like the
ty
in-arg eventually gets down toto_simm32
here which just checks ifty
is less than 64 bits, and skips an "is sign extendable" check if so; but it looks like this may actually be unnecessaryYeah I think we could just always do the "is sign extendable" check and it would be equivalent.
sunfishcode created PR review comment:
Ah, I see. Fwiw, I'd find
=ty
more intuitive syntax for this than<ty
.
fitzgen created PR review comment:
Whoops, good catch!
fitzgen submitted PR review.
fitzgen submitted PR review.
fitzgen created PR review comment:
I plan to add a CI check for whether the ISLE code is up-to-date, will implement that soon.
I think it would be easier to add a
rebuild-isle
feature tocranelift-codegen-meta
than to do all of that with the hashing and everything.
fitzgen submitted PR review.
fitzgen created PR review comment:
Yes, expanding formatting is something to do eventually, but I don't think it needs to block this PR.
fitzgen submitted PR review.
fitzgen created PR review comment:
Yes, that's the plan.
cfallin submitted PR review.
cfallin created PR review comment:
I think, at least the way I'm imagining it, the rebuild (
rebuild-isle
feature sounds good, I guess that's an implementation strategy for anupdate-isle.sh
or moral equivalent?) and the local-development-loop check (using hashing or something else) are orthogonal.Basically, (i) the rebuild is an explicit action that the user invokes in order to update the in-tree generated source, and (ii) hashing and checking in
build.rs
is one strategy of ensuring that a local build doesn't succeed unless the user does this explicit update.My thinking comes from two principles -- first, that we shouldn't have a setup where an edit to a
.isle
file is ignored during a rebuild if one forgets to do a manual step (otherwise, we're bound to waste time debugging why a change didn't make a difference...) and second, that it's kind of an anti-pattern to have a build step that automatically updates a file insrc/
, without the user asking for it. So from those two principles, we have that (i) there should be a separate update step, and (ii) we should check that it was done.Possibly those aren't principles that we want, but they seem to me to be a reasonable starting place; thoughts?
fitzgen updated PR #3506 from isle
to main
.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
Can these be inlined into the generated code? That should reduce compile times and improve debug mode performance. (The latter is important for cg_clif as CI compiles rustc using cg_clif and then builds a sysroot using it as test.)
bjorn3 submitted PR review.
bjorn3 created PR review comment:
This is a codegen regression, right?
bjorn3 created PR review comment:
There is a lot of inefficiencies in the generated code. While an optimizer can fix it, this only happens in release mode and increases build time.
cfallin submitted PR review.
cfallin created PR review comment:
That would be a major rearchitecting of the ISLE DSL compiler -- it would require us to carry around Rust code as strings and inject it, and have a general language feature for this, which IMHO at least is a thing we want to avoid.
The basic idea of the integration between DSL code and Rust code is that we're able to define helpers like these, and then call them; blending the boundary further is I think a recipe (as it were) for complexity and pain...
cfallin submitted PR review.
cfallin created PR review comment:
It's true that the DSL compiler has a simple emission strategy, but I don't think this sort of thing specifically actually results in worse code. Basically we are taking a compound expression and pulling it apart into separate let-bindings; afaik the MIR from this should be identical.
fitzgen submitted PR review.
fitzgen created PR review comment:
=variable
is already used for unifying two variables, eg(rule (has_type ty (ixor x =x)) (imm ty 0))
cfallin created PR review comment:
As a higher-level point, while I definitely understand the request, I don't think that "optimize for performance when the inliner doesn't inline anything" is a reasonable principle from a broader point of view: it is basically saying that we cannot use abstraction or helper functions, because the toolchain might have a bad worst-case behavior. IMHO it's reasonable to assume we can write single-line helpers, and sacrificing expressivity (even basic principles like extracting helper functions) in order to speed up the software in a debug build seems like a poor tradeoff, and one that is better fixed by finding ways to build software in fast-but-lightly-optimized ways (which is, incidentally, what
cg_clif
is meant to solve if I'm not mistaken!).
cfallin submitted PR review.
fitzgen submitted PR review.
fitzgen created PR review comment:
I don't think so: these used to be the
mov $$0, %reg
s in the branches and correctly get turned intoxor %reg, %reg
now.
fitzgen edited PR review comment.
fitzgen edited PR review comment.
fitzgen submitted PR review.
fitzgen created PR review comment:
I don't think that "optimize for performance when the inliner doesn't inline anything" is a reasonable principle
Strong agree
cfallin submitted PR review.
cfallin created PR review comment:
@fitzgen since we're hoping it will mostly be a lower-level thing that we build on top of, we could make the syntax more explicit, maybe: what do you think about something like
(eval ...)
or(matcher-input ...)
?(Basically, I too would be happy to see
<ty
die; it was literally just the first punctuation character I thought of and I wish I had chosen something better initially!)
fitzgen updated PR #3506 from isle
to main
.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
MIR doesn't do copy propagation. Every assignment results in a copy at MIR level.
Current code: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=8cf97ba023221ffafccb62ab89231fab
Excerpt:
bb0: { _5 = move _2; // scope 0 at src/lib.rs:21:22: 21:26 _6 = move _3; // scope 1 at src/lib.rs:22:22: 22:26 _7 = _4; // scope 2 at src/lib.rs:23:22: 23:26 discriminant(_8) = 0; // scope 3 at src/lib.rs:25:19: 25:50 _12 = &mut (*_1); // scope 4 at src/lib.rs:26:39: 26:42 _13 = move _5; // scope 4 at src/lib.rs:26:44: 26:54 _15 = &_8; // scope 4 at src/lib.rs:26:56: 26:64 _14 = _15; // scope 4 at src/lib.rs:26:56: 26:64 _16 = move _6; // scope 4 at src/lib.rs:26:66: 26:76 _17 = _7; // scope 4 at src/lib.rs:26:78: 26:88 _11 = constructor_shift_r::<C>(move _12, move _13, move _14, move _16, move _17) -> bb1; // scope 4 at src/lib.rs:26:19: 26:89 // mir::Constant // + span: src/lib.rs:26:19: 26:38 // + literal: Const { ty: for<'r, 's, 't0> fn(&'r mut C, Type, &'s ShiftKind, Reg, &'t0 Imm8Reg) -> std::option::Option<Reg> {constructor_shift_r::<C>}, val: Value(Scalar(<ZST>)) } }
With copies eliminated: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=61312e466966b68bd721e61f98cc62e3
bb0: { _8 = &mut (*_1); // scope 0 at src/lib.rs:21:39: 21:42 _9 = move _2; // scope 0 at src/lib.rs:21:44: 21:48 _19 = const constructor_sar::<C>::promoted[0]; // scope 0 at src/lib.rs:21:50: 21:82 _11 = _19; // scope 0 at src/lib.rs:21:50: 21:82 _10 = _11; // scope 0 at src/lib.rs:21:50: 21:82 _12 = move _3; // scope 0 at src/lib.rs:21:84: 21:88 _13 = _4; // scope 0 at src/lib.rs:21:90: 21:94 _7 = constructor_shift_r::<C>(move _8, move _9, move _10, move _12, move _13) -> bb1; // scope 0 at src/lib.rs:21:19: 21:95 // mir::Constant // + span: src/lib.rs:21:19: 21:38 // + literal: Const { ty: for<'r, 's, 't0> fn(&'r mut C, Type, &'s ShiftKind, Reg, &'t0 Imm8Reg) -> std::option::Option<Reg> {constructor_shift_r::<C>}, val: Value(Scalar(<ZST>)) } }
Something that would probably help a lot more though is avoiding
?
and instead usingmatch
.?
desugars into several function calls:bb1: { _7 = <Option<Reg> as Try>::branch(move _8) -> bb2; // scope 1 at src/lib.rs:22:19: 22:68 // mir::Constant // + span: src/lib.rs:22:19: 22:68 // + literal: Const { ty: fn(std::option::Option<Reg>) -> std::ops::ControlFlow<<std::option::Option<Reg> as std::ops::Try>::Residual, <std::option::Option<Reg> as std::ops::Try>::Output> {<std::option::Option<Reg> as std::ops::Try>::branch}, val: Value(Scalar(<ZST>)) } } bb2: { _15 = discriminant(_7); // scope 1 at src/lib.rs:22:19: 22:68 switchInt(move _15) -> [0_isize: bb3, 1_isize: bb5, otherwise: bb4]; // scope 1 at src/lib.rs:22:19: 22:68 } bb3: { _18 = move ((_7 as Continue).0: Reg); // scope 1 at src/lib.rs:22:19: 22:68 _6 = move _18; // scope 6 at src/lib.rs:22:19: 22:68 _19 = move _6; // scope 2 at src/lib.rs:23:17: 23:24 ((_0 as Some).0: Reg) = move _19; // scope 2 at src/lib.rs:23:12: 23:25 discriminant(_0) = 1; // scope 2 at src/lib.rs:23:12: 23:25 goto -> bb6; // scope 2 at src/lib.rs:23:5: 23:25 } bb4: { unreachable; // scope 1 at src/lib.rs:22:19: 22:68 } bb5: { _0 = <Option<Reg> as FromResidual<Option<Infallible>>>::from_residual(move _17) -> bb6; // scope 4 at src/lib.rs:22:19: 22:68 // mir::Constant // + span: src/lib.rs:22:67: 22:68 // + literal: Const { ty: fn(std::option::Option<std::convert::Infallible>) -> std::option::Option<Reg> {<std::option::Option<Reg> as std::ops::FromResidual<std::option::Option<std::convert::Infallible>>>::from_residual}, val: Value(Scalar(<ZST>)) } } bb6: { return; // scope 0 at src/lib.rs:24:2: 24:2 }
cfallin submitted PR review.
cfallin created PR review comment:
OK, interesting. Echoing the point I made above in another thread, though, this seems to me to be more a problem that debug builds of Rust do no optimization whatsoever (and this is a problem for any Rust code that is written with abstractions or redundancies that optimize away), and less that our particular DSL codegen produces code that has optimizable parts.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
Replacing
expr?
withif let Some(x) = expr { x } else { return None; }
should be an easy change in the codegen. The other suggested changes are not worth it I guess, though they will help with improving build time.
sunfishcode submitted PR review.
sunfishcode created PR review comment:
I agree that this kind of thing could end up being significant, especially as we add more patterns to isle and the generated files grow. However, optimizations like these complicate the generator, and right now, keeping that code simple is particularly valuable, as the code is new and still rapidly changing.
As ISLE matures, we add more patterns, and the generated files become large enough that effects like this are measurable, I expect it'll make sense to revisit this.
fitzgen submitted PR review.
fitzgen created PR review comment:
simm32
immediates no longer need input parameters, fwiw. They were the only use of input args to extractors in all of the lowering code.One thing I was thinking, and maybe this is wild, was to always have extractors use
[...]
and always have expressions use(...)
but this doesn't really solve the case where a simple, non-operation expression is an input.Or maybe, since we aren't using input polarity arguments for extractors anymore, we simply don't need them and can remove them from the language?
fitzgen submitted PR review.
fitzgen created PR review comment:
So from those two principles, we have that (i) there should be a separate update step, and (ii) we should check that it was done.
Agreed with these principals.
I'm not convinced we have to do more than what this PR currently does in this PR though.
Like I think there is a separate chunk of work that removes all of
cranelift-codegen-meta
fromcranelift-codegen
's dependency tree and does the all things you're talking about. But for now, the ISLE integration basically does what we already do for the rest ofcranelift-codegen-meta
, and I think that is fine for landing the initial ISLE integration.
fitzgen updated PR #3506 from isle
to main
.
cfallin submitted PR review.
cfallin created PR review comment:
Ah, OK, possibly I'm not understanding then -- from what I can tell in this PR, the
rebuild-isle
feature is non-default, and if I (i) do a clean git-clone, (ii) modify some ISLE source, and (iii)cargo build
, without specifying the feature or doing anything else, my local build doesn't pick up the new ISLE changes -- is that right? Or is there some plumbing somewhere that takes care of that?This is basically the case that motivates what I had suggested above -- I think that we need a check not just in CI, but locally, that the checked-in code matches what would have been generated (but only actually overwrite if the user opts-in, with a feature or script or whatnot). To do otherwise seems to me to invite lots of confusion the first time a new contributor tries to add a lowering and wonders why it doesn't work :-)
sunfishcode submitted PR review.
sunfishcode created PR review comment:
It may be different in terms of what's actually going on in the DSL, however as someone just reading the code, this
=x
and the<ty
above kinda feel like they're doing the same conceptual thing. Whether it's a type or a variable, the prefix means "don't bind a new thing here, reuse an existing binding". So I wonder if it would work to use prefix-=
syntax for both.
fitzgen submitted PR review.
fitzgen created PR review comment:
Yeah if you don't build with
--features rebuild-isle
then it won't rebuild ISLE or tell you to use that feature right now.I think this is fine for this PR (since we do check for and catch this in CI in this PR) and then in a future PR we can redo the whole paradigm for how we do meta, compiler compile time.
cfallin submitted PR review.
cfallin created PR review comment:
OK; I agree it's fine for now (first PR) but it seems like something that's likely to bite us pretty soon after, just as soon as others start to use it. I suspect it could be addressed in ~50 lines or so of code in
build.rs
, without requiring us to build the ISLE compiler in the default build, just hashing files and either reading/comparing or writing to a checked-in "manifest". I'm happy to do so once the initial thing merges. In the meantime could you add a TODO somewhere with a tracking issue?Thanks for the patience in working this one out!
fitzgen submitted PR review.
fitzgen created PR review comment:
fitzgen updated PR #3506 from isle
to main
.
fitzgen updated PR #3506 from isle
to main
.
fitzgen updated PR #3506 from isle
to main
.
fitzgen updated PR #3506 from isle
to main
.
fitzgen updated PR #3506 from isle
to main
.
fitzgen updated PR #3506 from isle
to main
.
bnjbvr submitted PR review.
bnjbvr created PR review comment:
Wouldn't there be a way to have very explicit syntax with another sub-expression or an annotation, instead of using an operator which significance has to be looked up in documentation and learned about? Ad-hoc syntax isn't self-explanatory, and as a matter of fact it is spurious extra complexity for newcomers, especially if it _can_ be avoided by using another more explicit syntax.
cfallin submitted PR review.
cfallin created PR review comment:
Agreed with @bnjbvr; I'd like to re-boost my suggestion above for
(matcher-input ...)
if we end up needing this feature still. I'm also hoping that, if it remains, we can keep its use confined to just whatever "standard library helpers" need it, if that, and keep it out of ordinary patterns entirely.Also agreed with @fitzgen that given that we found a way to avoid it here, maybe we can remove it; it nudges the language more toward "general-purpose constraint solver" (ie the Prolog direction) and less toward "patterns of instructions", when the latter is really the simplicity and approachability we're going for, so with whatever influence I still have on the language design, I'd like to push to undo my mistake :-)
fitzgen submitted PR review.
fitzgen created PR review comment:
For sure. I'm not going to deal with it right now, in this PR, but if we don't end up removing this feature from the DSL completely, we can use
(eval ...)
or(matcher-input ...)
or something.
fitzgen updated PR #3506 from isle
to main
.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
Should it be versioned together with cranelift?
fitzgen updated PR #3506 from isle
to main
.
fitzgen updated PR #3506 from isle
to main
.
fitzgen updated PR #3506 from isle
to main
.
fitzgen updated PR #3506 from isle
to main
.
fitzgen updated PR #3506 from isle
to main
.
fitzgen updated PR #3506 from isle
to main
.
fitzgen updated PR #3506 from isle
to main
.
fitzgen updated PR #3506 from isle
to main
.
fitzgen has marked PR #3506 as ready for review.
alexcrichton submitted PR review.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Is this step still necesary now that rustfmt is baked in?
alexcrichton created PR review comment:
I was poking around with a lowering that used this locally, and I think that this may not have been listed in the right order? I think that
Idx
should come after thevalue_array_2
, right?
alexcrichton created PR review comment:
FWIW I don't think this will run on oss-fuzz unless it's in the top-level directory by default (but we could modify oss-fuzz to build here too if desired)
alexcrichton created PR review comment:
I ended up having this -- https://github.com/alexcrichton/wasmtime/commit/d0f2bf4c140730566d00319def57fa1bdbc27269 -- to get matchers on
insertlane
working (although I'm not 100% sure it was correct)
cfallin submitted PR review.
cfallin submitted PR review.
cfallin created PR review comment:
Just a small thing, but unconditionally emitting terminal control codes without tty detection is kind of unfortunate; e.g. sometimes I pipe compiler output to a file or page through it with
less
. Also someone who is actually using a Braille screenreader would be broken by this option. Can we make this opt-in via an environment variable or Cargo feature?
cfallin created PR review comment:
Idle thought: it might make sense to namespace the x86 MachInst constructors all to
(x86_rotl ...)
,(x86_add ...)
, etc. -- then it's clear that we're lowering into a MachInst and not into some other intermediate form. If we did this it wouldn't have to be with this PR but we'd want to do it soon to avoid more find/replace pain later. Thoughts?
cfallin created PR review comment:
This can change to the Wasmtime repository now I think?
cfallin created PR review comment:
That's not a bad idea; @fitzgen can we call this "isle 0.78.0" along with Cranelift? We should make sure the version-bumping logic in
publish.rs
updates the isle/islec crates as well.
cfallin created PR review comment:
Is this file leftover from an earlier generated-code setup? I don't see where it's generated or used (but possibly I'm missing something)... and
src/clif.isle
is also used to generate the x64-specific lowering code so I don't think we need this separate machine-independent one?
fitzgen submitted PR review.
fitzgen created PR review comment:
This isn't emitting the terminal control codes, there is a separate config for colors, it is just whether we get pure-text error messages or whether it includes the source context snippets and unicode box characters. It works just fine piped to
less
or into a file.
fitzgen submitted PR review.
fitzgen created PR review comment:
I had similar thoughts of using
namespace:instruction
like(clif:iadd ...)
and(x86:add ...)
. Happy to bike shed in a follow up PR.
fitzgen submitted PR review.
fitzgen created PR review comment:
Whoops yeah this is unused.
fitzgen updated PR #3506 from isle
to main
.
cfallin submitted PR review.
cfallin created PR review comment:
Ah, OK, that's much better, sorry for misunderstanding! Maybe a comment here to the effect of "Ensure
miette
emits source snippets, even when the output is not a tty (note there are no terminal control codes in 'graphical' output)" or somesuch would help?
fitzgen updated PR #3506 from isle
to main
.
fitzgen submitted PR review.
fitzgen created PR review comment:
I actually don't think we should run this fuzz target in oss-fuzz. It is useful to run locally a bit for folks poking on the ISLE compiler, but the ISLE compiler doesn't need to be hardened in the same way that Wasmtime does.
fitzgen submitted PR review.
fitzgen created PR review comment:
Yes, since the
rustfmt
ing is happening on a best-effort basis, and ifrustfmt
isn't on$PATH
it doesn't break the build.
fitzgen created PR review comment:
That looks correct to me. Do you want to make a PR after this one merges?
fitzgen submitted PR review.
fitzgen updated PR #3506 from isle
to main
.
cfallin submitted PR review.
cfallin created PR review comment:
Ah, actually, reading this again, I note 'unicode box characters'. I do think that making this opt-in is actually an important accessibility thing; if
miette
went to the trouble of supporting screenreaders, we should not undo that work.
fitzgen submitted PR review.
fitzgen created PR review comment:
The problem is that their detection algorithm is broken and the errors are basically unusable without
force_graphical(true)
for me, the main person reading these errors.
cfallin created PR review comment:
I don't think it's either-or, though; could we just add a Cargo feature? What I'm envisioning is
if cfg!(feature = "miette-errors-graphical") {
...}
around this. Or an environment-variable if that's easier...
cfallin submitted PR review.
fitzgen submitted PR review.
fitzgen created PR review comment:
I'm not keen to add another speed bump to my own development process by requiring that I enable another env var or cargo feature that I'll forget most of the time.
How about we leave it as is for now, I file a bug upstream in
miette
, and then we remove this hack oncemiette
fixes the root cause?
cfallin submitted PR review.
cfallin created PR review comment:
Yep, that sounds like a reasonable compromise! Just a FIXME comment here noting said issue would be good, then I'm happy :+1:
fitzgen updated PR #3506 from isle
to main
.
fitzgen merged PR #3506.
fitzgen submitted PR review.
fitzgen created PR review comment:
Last updated: Dec 23 2024 at 12:05 UTC