This is an archive of the discontinued LLVM Phabricator instance.

[clang-doc] Update BitcodeReader to use llvm::Error
ClosedPublic

Authored by juliehockett on Jul 3 2018, 5:45 PM.

Details

Summary

Replace booleans with the more descriptive llvm::Error or llvm::Expected<T>

Diff Detail

Event Timeline

juliehockett created this revision.Jul 3 2018, 5:45 PM
Eugene.Zelenko added inline comments.
clang-tools-extra/clang-doc/BitcodeReader.cpp
14

Please include Support/Error.h and utility

clang-tools-extra/clang-doc/BitcodeReader.h
26

Please also include Support/Error.h

juliehockett marked 2 inline comments as done.
ioeric removed a reviewer: ioeric.Aug 6 2018, 8:05 AM

Should there be any tests associated with these changes?

clang-tools-extra/clang-doc/BitcodeReader.cpp
308

Probably not important or it's just me being picky/dumb, but is this exit the intended behavior of this program when reaching this function? If so, should the exit also be expected even when using llvm::Error?

This also applies to the other times exit() is called in these add functions.

311

Is it necessary to change the return value to llvm::Error for what were originally void functions?

juliehockett marked 2 inline comments as done.Aug 9 2018, 2:37 PM
juliehockett added inline comments.
clang-tools-extra/clang-doc/BitcodeReader.cpp
308

Yes, the point of this change is to get rid of that behavior and return a useful error. That's why we change it to return that error instead of void

311

See above

juliehockett marked 2 inline comments as done.Aug 9 2018, 2:39 PM

Should there be any tests associated with these changes?

There are, this is just supposed to be a NFC so the tests don't need to change. They all pass :)

leonardchan accepted this revision.Aug 9 2018, 2:43 PM
This revision is now accepted and ready to land.Aug 9 2018, 2:43 PM

You could use Differential revision: <review URL> in commit description to close review automatically.