[llvm-commits] [PATCH] YAML I/O

[Michael Spencer]

On Mon, Aug 6, 2012 at 12:17 PM, Nick Kledzik <kledzik at apple.com> wrote:
> Attached is a patch for review which implements the Yaml I/O library I proposed on llvm-dev July 25th.
>
>
>
>
> The patch includes the implementation, test cases, and documentation.
>
> I've included a PDF of the documentation, so you don't have to install the patch and run sphinx to read it.
>
>
>
> There are probably more aspects of yaml we can support in YAML I/O, but the current patch is enough to support my needs for encoding mach-o as yaml for lld test cases.
>
> I was initially planning on just adding this code to lld, but I've had two requests to push it down into llvm.
>
> Again, here are examples of the mach-o schema and an example mach-o document:
>
>
>
>
>
>
> -Nick

Cool, thanks for writing this. Much less verbose than using the raw
yaml interface.

I have a few general comments about the patch. The first is style. It
needs to follow the LLVM style, this includes Capitalized variable
names everywhere and &/* on the right (you got most of them, but there
are a few left).

I also feel that not having access to the yaml::Nodes makes it harder
to give good diagnostics for things not modeled by YAMLIO.

Another issue is that this doesn't seem to be able to pick up parsing
in the middle of a stream. This is needed if users don't want to use
YAMLIO to parse everything in the file. You should be able to give it
a yaml::Node* and have it go from there.

I've yet to do a full review on the code, but I did spot this issue.

+
+private:
+  llvm::yaml::Stream                   *_stream;
+  llvm::error_code                      _error;
+  llvm::BumpPtrAllocator                _nodeAllocator;
+  llvm::SmallString<128>                _stringStorage;
+  std::vector<HNode*>                   _documentRoots;
+  llvm::SourceMgr                       _srcMgr;
+  llvm::yaml::document_iterator         _docIterator;
+  std::vector<bool>                     _bitValuesUsed;
+  HNode                                *_currentNode;
+};

_stringStorage here is used as a target for ScalarNode::getValue. The
problem with this is that getValue clears out the storage passed to it
if it actually uses the storage. This would invalidate all other
values gotten this way.

I believe the proper solution to this would be to somehow allow
getValue to target a BumpPtrAllocator. This would give us exactly the
semantics we want.

I'm still reviewing the rest of the code, but I wanted to get this to you soon.

- Michael Spencer