Stream: git-wasmtime

Topic: wasmtime / Issue #1299 [wiggle] Impl different formatters...

Wasmtime GitHub notifications bot (Mar 12 2020 at 15:06):

sunfishcode commented on Issue #1299:

I'd like to suggest a different possible approach here: instead of different way to represent flags as integers, we format flags with human-readable names.

For example, instead of an fdflags value being '0b10001 or 0x11 or 041, we print it as append|sync (0x11)`. Then most people will just read the names, and we can just have one formatting method.

That will get pretty verbose with big flag words that frequently have many flags set, like rights, but I propose as a first iteration, being overly verbose with flag names is still better than numbers that I have to mentally map to flag names :-). And as a second iteration, perhaps we could declare RIGHTS_REGULAR_FILE_BASE etc. in witx somehow (which we should probably do anyway), and then formatting code could compute which *_BASE xor'd with the given flags word has the least hamming distance and print the base with difference, eg. tty&!write|tell or so.

Thoughts?

Wasmtime GitHub notifications bot (Mar 12 2020 at 15:07):

sunfishcode edited a comment on Issue #1299:

I'd like to suggest a different possible approach here: instead of different way to represent flags as integers, we format flags with human-readable names.

For example, instead of an fdflags value being 0b10001 or 0x11 or 041, we print it as append|sync (0x11). Then most people will just read the names, and we can just have one formatting method.

That will get pretty verbose with big flag words that frequently have many flags set, like rights, but I propose as a first iteration, being overly verbose with flag names is still better than numbers that I have to mentally map to flag names :-). And as a second iteration, perhaps we could declare RIGHTS_REGULAR_FILE_BASE etc. in witx somehow (which we should probably do anyway), and then formatting code could compute which *_BASE xor'd with the given flags word has the least hamming distance and print the base with difference, eg. tty&!write|tell or so.

Thoughts?

Wasmtime GitHub notifications bot (Mar 12 2020 at 15:32):

kubkon commented on Issue #1299:

Oh, sounds intriguing! I like it! I'm not sure how easy it would be but I reckon it's worth a try!

Wasmtime GitHub notifications bot (Mar 12 2020 at 17:17):

github-actions[bot] commented on Issue #1299:

Subscribe to Label Action

This issue or pull request has been labeled: "w", "a", "s", "i"

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.

Wasmtime GitHub notifications bot (Mar 12 2020 at 19:46):

alexcrichton commented on Issue #1299:

FWIW I think the bitflags crate in Rust does what @sunfishcode is mentioning, and it's pretty slick and would recommend having that here as well for the default.

Wasmtime GitHub notifications bot (Mar 12 2020 at 19:52):

kubkon commented on Issue #1299:

So I thought about using bitflags crate originally but it requires another dependency and it was awkward to work with previously, as the user of wiggle would effectively be forced to include bitflags as a dependency in the crate they’d use wiggle in. Now, I haven’t checked if that’s still the case. If it’s not, than I’m all for using it as it would drastically simplify generation of a couple of data types (flags and ints in particular).

Wasmtime GitHub notifications bot (Mar 12 2020 at 20:08):

alexcrichton commented on Issue #1299:

Oh sure yeah to be clear I don't think we should use the bitflags crate directly, but just wanted to point out some prior art if it's helpful.

Wasmtime GitHub notifications bot (Mar 12 2020 at 20:15):

kubkon commented on Issue #1299:

Ah, gotcha! :-)

Wasmtime GitHub notifications bot (Mar 12 2020 at 22:41):

kubkon commented on Issue #1299:

@sunfishcode @alexcrichton Flags is now by default formatted similarly to how bitflags crate does it, namely, dsync|append (0x11). In case we're dealing with an empty set, we get empty (0x0). Because of this, any Octal, LowerHex, etc., formatters are redundant now.

Furthermore, while here, I've rewritten EMPTY_FLAGS and ALL_FLAGS (where the former means 0x0 and the latter is the union of all possible values) to be const fn empty() and const fn all() where the latter is an expanded union of primitive representation values out of a macro. This is again largely inspired by the bitflags crate.

Wasmtime GitHub notifications bot (Mar 13 2020 at 09:16):

kubkon commented on Issue #1299:

\o/

Can you add a test in tests/flags.rs which does .to_string() on some flags values and asserts that it produces the strings expected?

Excellent suggestion, I've added a testcase for this just now :-)

Last updated: Nov 22 2024 at 17:03 UTC