Idea: Adding `from_string` to enum metafunction #1185

tsoj · 2024-07-28T23:29:44Z

So that this works:

piece := Piece::from_string("pawn");
assert(piece == pawn);

Could something like this make sense?

hsutter · 2024-07-30T06:50:50Z

Thanks for your pull request! It looks like this may be your first contribution to cppfront. I'll ping you by email to send the Contributor License Agreement (CLA) to your email, and once it's signed I can look at your pull request. Thanks again for your contribution.

…dling Changed failures from unconditional abort to a type_safety violation (which defaults to abort but allows a customizable handler) because it really is a precondition violation so a contract makes sense

hsutter · 2024-07-30T23:36:55Z

Thanks! This seems like a good idea. I've taken a pass over it and updated the PR. The only reason I didn't merge it is that it's labeled "Draft"... ok to merge?

tsoj · 2024-07-30T23:39:34Z

Yeah its ready, I've only wondered if there is a good way to extend this to bitwise enums, but maybe that can be a seperate PR when someone actually needs it.

hsutter · 2024-07-31T00:03:00Z

I've only wondered if there is a good way to extend this to bitwise enums

I did that in my update to this PR, just by removing the check to prevent generating it for flag enums. It seems to work fine for any named enumerator, including named enumerators that are combinations of others? I added the test case:

f_from_string := file_attributes::from_string("cached_and_current");
std::cout << "f_from_string is " << f_from_string.to_string() << "\n";

which prints:

f_from_string is (cached, current, cached_and_current)

That seems correct, right -- two bits set?

tsoj · 2024-07-31T03:28:01Z

Ah okay, I missed that. I was mostly thinking that now the output of to_string isn't always a valid input for from_string (e.g "(cached, current, obsolete, cached_and_current)") which is a bit awkward.

I made a second branch where I added this, but it's a bit messy, so maybe it's not the best idea to add it right now.

hsutter · 2024-07-31T19:32:29Z

now the output of to_string isn't always a valid input for from_string (e.g "(cached, current, obsolete, cached_and_current)") which is a bit awkward.

Ah, good point! Working on it...

hsutter · 2024-07-31T20:51:04Z

OK, how does this look -- I think it's ready to merge, review please?

gregmarr · 2024-07-31T23:20:53Z

include/cpp2regex.h

@@ -1555,6 +1556,26 @@ if (((*this) & perl_code_syntax_in_classes) == perl_code_syntax_in_classes) {_re
 return cpp2::move(_ret) + ")"; 
 }

+[[nodiscard]] auto expression_flags::from_string(cpp2::impl::in<std::string_view> s) -> expression_flags{


@hsutter Why do all of these lines start at column 0? There is leading whitespace in the metafunction. Is it because the whitespace is tabs, and those are being stripped somehow?

Good question, I don't know offhand.

gregmarr · 2024-07-31T23:25:45Z

include/cpp2util.h

+        return std::isspace(str[pos]) || str[pos] == ',';
+    };
+
+    while( pos < std::ssize(str) ) {


nitpick, if you care: style difference between this while and the next three while/if, spaces inside parens here and not below, space after keyword below and not here.

gregmarr · 2024-07-31T23:46:50Z

regression-tests/pure2-enum.cpp2

@@ -117,4 +119,10 @@ main: () = {
        is (cpp2::has_flags(f2))      = "includes all f2's flags ('cached' and 'current')";
        is _                          = "something else";
    } << "\n";
+
+    f_from_string := file_attributes::from_string("cached_and_current");
+    std::cout << "f_from_string is " << f_from_string.to_string() << "\n";


It bothers me slightly that both of these:

file_attributes::to_string(file_attributes::from_string("cached_and_current")); (file_attributes::cached | file_attributes::current).to_string()

result in this:

"(cached, current, cached_and_current)"

I'm wondering if a bitwise enum to_string() should be a single combined value if it exactly matches a combined value, and then if it doesn't, only include the bare flags.

That would make the first ones result in "cached_and_current". Then (file_attributes::current | file:attributes.obsolete).to_string() would be (current, obsolete).

Also, should to_string() include file_attributes::?

Thanks!

It bothers me slightly that both of these:

file_attributes::to_string(file_attributes::from_string("cached_and_current")); (file_attributes::cached | file_attributes::current).to_string()

result in this:

"(cached, current, cached_and_current)"

I'm wondering if a bitwise enum to_string() should be a single combined value if it exactly matches a combined value, and then if it doesn't, only include the bare flags.

What I've been doing with to_string is emitting all of the named values that are matched. That way it's consistent with has.

Also, should to_string() include file_attributes::?

to_string has a this parameter, from_string doesn't so it needs the qualification.

What I've been doing with to_string is emitting all of the named values that are matched. That way it's consistent with has.

My "nightmare scenario" is something like the creating an enum for the file permissions in <sys/stat.h>, and then doing ALLPERMS.to_string() and getting this:

"(S_ISUID, S_ISGID, S_ISTXT, S_ISVTX, S_IRWXU, S_IRUSR, S_IWUSR, S_IXUSR, S_IREAD, S_IWRITE, S_IEXEC, S_IRWXG, S_IRGRP, S_IWGRP, S_IXGRP, S_IRWXO, S_IROTH, S_IWOTH, S_IXOTH, ACCESSPERMS, ALLPERMS, DEFFILEMODE)"

to_string has a this parameter, from_string doesn't so it needs the qualification.

To clarify, I meant in the output string. It was just a thought, not for computer consumption, but more for human consumption. I was thinking that the user always has to spell it as file_attributes::current, unless they're inside the class, so would it make sense for to_string() to produce that? That would make from_string() more difficult, I guess.

Related to that, would it make sense to use | instead of , so that it makes actually usable as code?

Related to that, would it make sense to use | instead of , so that it makes actually usable as code?

👍🏻 I was thinking of this myself when I used flag_enums a while ago.

Thanks! I just pushed a commit to change , to | for flag enums.

Allowing qualification is doable but just makes reading and parsing harder. A poor person's way to make it a qualified string would be to prepend flag_name:: and then replace every | with | flag_name::... perhaps it's worth adding a to_string_qualified that just calls to_string and then does that, and a from_string_qualified that replaces all flag_name:: with nothing and calls from_string...?

Indeed, it could be a thing. Python for example has __repr__ and __str__ which cover both of these use-cases.

I'm not sure if there are use cases here, just things that came to mind as I was reading it.

Maybe the "a single combined value if it exactly matches a combined value, and then if it doesn't, only include the bare flags" would be a separate function and it could have a qualified option?

source/reflect.h2

And fixed bug to correct error case `break`

to_string/from_string round-trip, don't enum_name::-qualify, and represent a list with comma separators to_code/from_code round-trip, do enum_name::-qualify, and represent a list with | separators

hsutter · 2024-08-02T21:53:07Z

OK, I've pushed a commit that now supports both:

to_string that round-trips with from_string, which uses unqualified names and , separators if it's a list, and
to_code that round-trips with from_code, which uses qualified:: names and | separators if it's a list

Please check? How does this look?

gregmarr · 2024-08-02T22:22:28Z

source/reflect.h2

-                                             "_ret += _comma + \"(e.name)$\"; _comma = \", \"; "
-                                         "}\n";
+                        to_string_impl += "    if (this & (e.name)$) == (e.name)$ { "
+                                                  "ret += sep + cpp2::to_string(prefix) + \"(e.name)$\"; sep = separator; "


This results in code like this:

if (((*this) & case_insensitive) == case_insensitive) {ret += sep + cpp2::to_string(prefix) + "case_insensitive";sep = separator;} if (((*this) & multiple_lines) == multiple_lines) {ret += sep + cpp2::to_string(prefix) + "multiple_lines";sep = separator;} if (((*this) & single_line) == single_line) {ret += sep + cpp2::to_string(prefix) + "single_line";sep = separator;}

This calls sep + cpp2::to_string(prefix) for every matched bit.

Could the function start something like this:

auto first = cpp2::to_string(prefix); auto next = separator + first; auto cur = &first;

and then for each bit

if (((*this) & case_insensitive) == case_insensitive) {ret += cur + "case_insensitive"; cur = &next;}

Add regress test case for to_code / from_code Update regression test and generated files

Signed-off-by: Herb Sutter <[email protected]>

tsoj added 2 commits July 29, 2024 01:21

Added from_string to enum metafunction

37535c0

Changed string to string_view

9a25f87

hsutter added the awaiting-cla label Jul 30, 2024

Changed string to string_view in test result now too

8587a6d

hsutter removed the awaiting-cla label Jul 30, 2024

Enable from_string for flag_enums too, and use contract violation han…

e426805

…dling Changed failures from unconditional abort to a type_safety violation (which defaults to abort but allows a customizable handler) because it really is a precondition violation so a contract makes sense

tsoj marked this pull request as ready for review July 30, 2024 23:37

Fix typo in regression test case

918584b

For flag_enums, let from_string take a "( list, of_multiple, flags )"

7c02229

gregmarr reviewed Jul 31, 2024

View reviewed changes

tsoj commented Aug 1, 2024

View reviewed changes

source/reflect.h2 Outdated Show resolved Hide resolved

hsutter added 2 commits August 1, 2024 14:40

Changed flag_enum to_string from (a,b) to (a|b)

654fff0

And fixed bug to correct error case `break`

Provide {to|from}_{string|code}

9a73208

to_string/from_string round-trip, don't enum_name::-qualify, and represent a list with comma separators to_code/from_code round-trip, do enum_name::-qualify, and represent a list with | separators

gregmarr reviewed Aug 2, 2024

View reviewed changes

hsutter added 3 commits August 2, 2024 18:39

Avoid repeated eval of to_string(prefix)

ab43b7d

Add regress test case for to_code / from_code Update regression test and generated files

Merge branch 'main' into enum_from_string

0a4bd0d

Signed-off-by: Herb Sutter <[email protected]>

Merge branch 'main' into enum_from_string

d3fb6bf

Signed-off-by: Herb Sutter <[email protected]>

hsutter merged commit fe989d4 into hsutter:main Aug 4, 2024
18 of 28 checks passed

Idea: Adding from_string to enum metafunction #1185

Idea: Adding from_string to enum metafunction #1185

Uh oh!

Conversation

tsoj commented Jul 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hsutter commented Jul 30, 2024

Uh oh!

hsutter commented Jul 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tsoj commented Jul 30, 2024

Uh oh!

hsutter commented Jul 31, 2024

Uh oh!

tsoj commented Jul 31, 2024

Uh oh!

hsutter commented Jul 31, 2024

Uh oh!

hsutter commented Jul 31, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hsutter Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregmarr Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hsutter commented Aug 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Idea: Adding `from_string` to enum metafunction #1185

Idea: Adding `from_string` to enum metafunction #1185

tsoj commented Jul 28, 2024 •

edited

Loading

hsutter commented Jul 30, 2024 •

edited

Loading

hsutter Aug 1, 2024 •

edited

Loading

gregmarr Aug 1, 2024 •

edited

Loading