This is an archive of the discontinued LLVM Phabricator instance.

[YAMLTraits] Fix mapping <none> value that followed by comments.
ClosedPublic

Authored by Higuoxing on Aug 4 2020, 12:26 AM.

Details

Summary

When mapping an optional value, if the value is <none> and followed
by comments, there will be a parsing error. This patch helps fix this
issue.

e.g.,

When mapping the following YAML,

Sections:
  - Name:  blah
    Type:  SHT_foo
    Flags: [[FLAGS=<none>]] ## some comments.

the raw value of ScalarNode is "<none> " rather than "<none>". We need
to remove the spaces.

Diff Detail

Event Timeline

Higuoxing created this revision.Aug 4 2020, 12:26 AM
Herald added a project: Restricted Project. · View Herald Transcript
Higuoxing requested review of this revision.Aug 4 2020, 12:26 AM
Higuoxing edited the summary of this revision. (Show Details)Aug 4 2020, 12:28 AM

Hmm. I do not know a simpler solution for this, so it is probably looks fine to me.
I wonder what others think though.

llvm/include/llvm/Support/YAMLTraits.h
1632–1634

This probably deserves a comment now.
Should we use rtrim(' ')? We do not need to trim the left side + trim by default trims too much it seems: " \t\n\v\f\r".

Higuoxing added inline comments.Aug 4 2020, 1:23 AM
llvm/include/llvm/Support/YAMLTraits.h
1631–1634

Another approach would be using getValue(SmallVectorImpl<char> &Storage).

/// Gets the value of this node as a StringRef.
///
/// \param Storage is used to store the content of the returned StringRef iff
///        it requires any modification from how it appeared in the source.
///        This happens with escaped characters and multi-line literals.
StringRef getValue(SmallVectorImpl<char> &Storage) const;
Flags: [[FLAGS='<none>']] can also be correctly parsed.
Higuoxing edited the summary of this revision. (Show Details)Aug 4 2020, 1:28 AM

@grimar knows this area better, so I'll leave him to do most of this reviewing. I think the description needs updating though. Currently it talks about fields written as "Flags: <none>", but I assume you mean it to be "Flags: [[FLAGS=<none>]]"?

Higuoxing edited the summary of this revision. (Show Details)Aug 4 2020, 1:34 AM
grimar added inline comments.Aug 4 2020, 1:45 AM
llvm/include/llvm/Support/YAMLTraits.h
1631–1634

I'd probably just stop on on rtrim( ) as it does exactly what we need here.

ScalarNode::getValue(SmallVectorImpl<char> &Storage) also handles single and double quotes,
what doesn't seem like something what we want to support.

Higuoxing updated this revision to Diff 282832.Aug 4 2020, 1:58 AM
Higuoxing marked an inline comment as done.

Address comments.

Thanks for reviewing!

grimar accepted this revision.Aug 4 2020, 2:07 AM

LGTM with a nit.

llvm/include/llvm/Support/YAMLTraits.h
1631–1634

nit: still missing a comment. I'd add something like:

// We use rtrim to ignore possible white spaces that might exist when a comment is present on the same line.

This revision is now accepted and ready to land.Aug 4 2020, 2:07 AM
Higuoxing updated this revision to Diff 282836.Aug 4 2020, 2:11 AM
Higuoxing marked an inline comment as done.

Address comments.