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

[Nick Kledzik]

On Aug 7, 2012, at 12:30 PM, Michael Spencer wrote:
> I also feel that not having access to the yaml::Nodes makes it harder
> to give good diagnostics for things not modeled by YAMLIO.
I debated about this.  I'd rather find out what kinds of yaml are not modeled and see if we can extend yaml I/O to support that (for instance, that is how I added BitValue).  I worry that it we make it too easy to drop down and get a yaml::Node, that that will be used instead of improving yaml I/O.   Also, we'd need some complementary solution for write out the yaml for those cases. 

Do you have some cases in mind?

> 
> 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.
That may work on the input side, but the output side needs to know indent level, etc.  

You mentioned the other day about using tags on the "---" token to denote which kind of document it was.  We'd definitely need to figure out how to model that.   The current yaml I/O patch only supports homogenous documents lists.  I need a way to model a stream that has different document types (as will be needed by lld test cases).


> 
> 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.
Hmm.  I was talking to Daniel Dunbar about this issue a while back and he thought most APIs that to a "storage" parameter would just append.  I glanced at ScalarNode::getValue() and thought is was appending, but on looking closer, it clearly does not.   Would it make sense to change that?  Or do you think there are clients that depend on getValue() clearing (then appending)?


> 
> 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