This is an archive of the discontinued LLVM Phabricator instance.

[dsymutil] Tablegenify option parsing
ClosedPublic

Authored by JDevlieghere on Oct 2 2019, 4:57 PM.

Details

Summary

This patch reimplements command line option parsing in dsymutil with Tablegen and libOption. The main motivation for this change is to prevent clashes with other cl::opt options defined in llvm. Although it's a bit more heavyweight, it has some nice advantages such as no global static initializers and better separation between the code and the option definitions.

I also used this opportunity to improve how dsymutil deals with incompatible options. Instead of having checks spread across the code, everything is now grouped together in verifyOptions. The fact that the options are no longer global means that we need to pass them around a bit more, but I think it's worth the tradeoff.

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.Oct 2 2019, 4:57 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 2 2019, 4:57 PM
thegameg accepted this revision.Oct 3 2019, 7:50 AM

I noticed a bunch of explicit llvm:: prefixes like llvm::Error, llvm::StringRef, etc. Did you intentionally leave that there?

Otherwise, this LGTM, thanks for the nice cleanup!

llvm/tools/dsymutil/dsymutil.cpp
161 ↗(On Diff #222934)

Would it make sense for all these error codes to be std::errc::invalid_argument?

233 ↗(On Diff #222934)

= std::move(*InputFiles);?

This revision is now accepted and ready to land.Oct 3 2019, 7:50 AM
friss accepted this revision.Oct 3 2019, 8:17 AM

This seems a little heavyweight, but it reads nicely and gets rid of global state. Go for it.

I noticed a bunch of explicit llvm:: prefixes like llvm::Error, llvm::StringRef, etc. Did you intentionally leave that there?

No, that's definitely unintentional. I'll fix the inconsistency in a follow-up commit! :-)

This revision was automatically updated to reflect the committed changes.