[PATCH] D21987: [XRay] Implement `llvm-xray extract`, start of the llvm-xray tool

Dean Michael Berris via llvm-commits llvm-commits at lists.llvm.org
Wed Oct 5 22:36:39 PDT 2016


dberris added inline comments.


> dblaikie wrote in xray-extract.cc:114
> pair<StringRef, error_code> looks a bit like it should be llvm::Error, no?

Ooh, nice -- I wasn't aware of `llvm::Error` before. I'll use that instead.

> dblaikie wrote in xray-extract.cc:118-119
> Branch-to-unreachable should be an assert instead, but chances are this should be a handled error case, right? Or is filename non-empty a precondition to this function?

Actually this should be an error, I'll change it.

> dblaikie wrote in xray-extract.cc:302-303
> Might be worth making the registerCommand into a "CommandRegistration" (since they'll all want to do it in a global init anyway, so it's not like you'll have other callers/uses of registerCommand except in idioms that all look like this global ctor):
> 
>   static CommandRegistration Unused(&Extract, [] { ... });

That's a really good point, I definitely like this style better too. :)

> dblaikie wrote in xray-extract.h:61-63
> Documentation comment, perhaps?
> 
> It's not clear, looking at the signature, what this might do (how could the output be provided in a StringRef? What would the ownership semantics be? Should this be called "convert" rather than "extract" if it maps from one format to another? Should this have an intermediate representation so that different input and output formats can be generically composed?)

Actually, this is vestigial (we don't define this function anymore).

https://reviews.llvm.org/D21987





More information about the llvm-commits mailing list