Page MenuHomePhabricator

[YAML] Support extended spellings when parsing bools.
ClosedPublic

Authored by njames93 on Dec 7 2020, 4:43 AM.

Details

Summary

Support all the spellings of boolean datatypes according to https://yaml.org/type/bool.html

Diff Detail

Event Timeline

njames93 created this revision.Dec 7 2020, 4:43 AM
njames93 requested review of this revision.Dec 7 2020, 4:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2020, 4:43 AM

The spec link doesn't seem to support AllowNumeric. Why do we have that feature?

The spec link doesn't seem to support AllowNumeric. Why do we have that feature?

Its disabled by default, It was added for partial support of a child of this, but thinking about it, it may be better to extract that behaviour out.

njames93 updated this revision to Diff 309984.Dec 7 2020, 12:09 PM

Removed AllowNumeric support

silvas accepted this revision.Dec 7 2020, 12:26 PM

Thanks. This LGTM.

llvm/unittests/Support/YAMLParserTest.cpp
356

Nit: I find these a little hard to scan. Can we separate the "true" and "false" cases with comments, and then sort within each one (it's not obvious to me how the current ones are sorted).

This revision is now accepted and ready to land.Dec 7 2020, 12:26 PM

Also I may need to investigate the pre-merge, some tests were failing, some looked like they involved yaml and are related to this. Keys called N were missing because the parser was treating them as a bool it seems. Will let it build again to be sure it is this patch causing the issue.

llvm/unittests/Support/YAMLParserTest.cpp
356

I'm a little lost in what you are asking. Comments where exactly?

silvas added inline comments.Dec 7 2020, 12:51 PM
llvm/unittests/Support/YAMLParserTest.cpp
356

Something like this. Sorting is whatever vim's :sort gave me.

// Test true values.
expectCanParseBool("ON", true);
expectCanParseBool("On", true);
expectCanParseBool("TRUE", true);
expectCanParseBool("True", true);
expectCanParseBool("Y", true);
expectCanParseBool("YES", true);
expectCanParseBool("Yes", true);
expectCanParseBool("on", true);
expectCanParseBool("true", true);
expectCanParseBool("y", true);
expectCanParseBool("yes", true);
expectCannotParseBool("1");

// Test false values.
expectCanParseBool("FALSE", false);
expectCanParseBool("False", false);
expectCanParseBool("N", false);
expectCanParseBool("NO", false);
expectCanParseBool("No", false);
expectCanParseBool("OFF", false);
expectCanParseBool("Off", false);
expectCanParseBool("false", false);
expectCanParseBool("n", false);
expectCanParseBool("no", false);
expectCanParseBool("off", false);
expectCannotParseBool("0");
356

(I think case-insensitive sort might be better honestly)

njames93 added inline comments.Dec 7 2020, 12:56 PM
llvm/include/llvm/Support/YAMLTraits.h
644

Ahh so this is causing the bool value set to be quoted, even the value isn't being used as a bool. Resulting in test case failures.

njames93 updated this revision to Diff 310004.Dec 7 2020, 1:18 PM

Fix other tests failing(hopefully).
Address order of tests and comments.

This revision was landed with ongoing or failed builds.Dec 12 2020, 4:50 AM
This revision was automatically updated to reflect the committed changes.