This is an archive of the discontinued LLVM Phabricator instance.

[yamlio] Allow parsing an entire mapping as an enumeration
ClosedPublic

Authored by sstwcw on Feb 22 2022, 3:19 PM.

Details

Summary

For when we want to change a configuration option from an enum into a
struct. The need arose when working on D119599.

Diff Detail

Event Timeline

sstwcw created this revision.Feb 22 2022, 3:19 PM
sstwcw requested review of this revision.Feb 22 2022, 3:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 22 2022, 3:19 PM
sstwcw added inline comments.Feb 22 2022, 3:22 PM
llvm/include/llvm/Support/YAMLTraits.h
66

As @curdeius pointed out, the function name enumInput is used instead of enumeration. I used that name in order to stress that it is only used for reading.

Looks ok for me, but I have no experience in this area.
I added some more reviewers, let's see what they say.

llvm/docs/YamlIO.rst
567

Reformat.

sstwcw updated this revision to Diff 411234.Feb 24 2022, 2:24 PM

format code

sstwcw marked an inline comment as done.Feb 24 2022, 2:25 PM

Could you explain more why you can't just update the configuration YAML to use the new format type? I'm not sure we really want to be getting into the business of supporting backwards-compatible YAMLIO code.

Could you explain more why you can't just update the configuration YAML to use the new format type? I'm not sure we really want to be getting into the business of supporting backwards-compatible YAMLIO code.

In clang-format we have an option which is currently an enum. Now @sstwcw wants to add some functionality to that, where we (as the main clang-format developers) said we have to convert the enum into a struct. But don't want to go the way (again) to add the enumerator Custom and then a second option with the struct. We would like to have the struct using the name of the current enum, but need to be compatible to the .clang-format files.

jhenderson added inline comments.Feb 28 2022, 12:54 AM
llvm/include/llvm/Support/YAMLTraits.h
1090

Does this REALLY need to be a macro? It looks like a function would suffice to me.

sstwcw updated this revision to Diff 412373.Mar 2 2022, 3:53 AM

Remove the macro.

Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 3:53 AM
sstwcw marked an inline comment as done.Mar 2 2022, 3:53 AM
sstwcw added inline comments.
llvm/include/llvm/Support/YAMLTraits.h
1090

It doesn't really need a macro.

sstwcw marked an inline comment as done.Mar 2 2022, 3:54 AM
jhenderson accepted this revision.Mar 2 2022, 10:33 PM

Thanks, LGTM.

This revision is now accepted and ready to land.Mar 2 2022, 10:33 PM
This revision was landed with ongoing or failed builds.Mar 13 2022, 9:47 PM
This revision was automatically updated to reflect the committed changes.