This is an archive of the discontinued LLVM Phabricator instance.

Simplify the use of CVTypeVisitor
ClosedPublic

Authored by zturner on May 16 2017, 10:30 AM.

Details

Summary

Similar to D33229, this patch is aimed at reducing boilerplate and simplifying usage.

The problem I'm trying to address here is that there is often a lot of code required to visit a type stream, or even a single record. You have to create a visitation pipeline. Then you have to stick a bunch of visitors in it. Then you have to invoke the right method on the pipeline. This is usually between 3 and 6 lines of code, with at least 3 different objects in play. But one of the most common usages is that you have a sequence of bytes, and you want to visit them as deserialized records. In order to make this work, you have to create a TypeVisitorCallbackPipeline and then put a TypeDeserializer on it to deserialize the records, and only then can you add your real callbacks to the visitation pipeline.

Since this is such a common use case, it would be nice if it were automatic. But we don't always want to deserialize records first. We might not even have any record data to deserialize in the first place (think Yaml -> PDB where we have Yaml describing the records, not bytes). So it can't be required.

At the same time, there is no fundamental reason why CVTypeVisitor has to be an object. It just happens to be one. We could just invoke a free free function and pass the visitor into the free function instead of constructing a CVTypeVisitor with one.

So this patch does all of the above. After this patch, the following code:

if (IO.outputting()) {
  codeview::TypeDeserializer Deserializer;

  codeview::TypeVisitorCallbackPipeline Pipeline;
  Pipeline.addCallbackToPipeline(Deserializer);
  Pipeline.addCallbackToPipeline(Context.Dumper);

  codeview::CVTypeVisitor Visitor(Pipeline);
  consumeError(Visitor.visitTypeRecord(Record));
}

turns into this:

if (IO.outputting())
  consumeError(codeview::visitTypeRecord(Record, Context.Dumper));

Diff Detail

Event Timeline

zturner created this revision.May 16 2017, 10:30 AM

Note that there is one instance of using CVTypeVisitor left that I couldn't get rid of in this patch. The one in RandomAccessVisitorTest. I have another patch which depends on this one which makes some fairly significantly change in this area, and after that patch we can get rid of that.

Once that happens, I plan to completely delete CVTypeVisitor (or at least hide it in the implementation file), and the only thing people will use is these newly introduced free functions.

amccarth edited edge metadata.May 16 2017, 1:41 PM

Having just figured out some of this pipeline stuff, I really appreciate making this easier to user.

llvm/include/llvm/DebugInfo/CodeView/CVTypeVisitor.h
51

I'm not a big fan of bool params, because it can be harder for people reading the call sites to know what they mean. How about making the Deserialize parameter an enum?

zturner updated this revision to Diff 99217.May 16 2017, 3:22 PM

Use enum instead of bool.

amccarth accepted this revision.May 16 2017, 3:45 PM

I'm happy.

This revision is now accepted and ready to land.May 16 2017, 3:45 PM
This revision was automatically updated to reflect the committed changes.