This is an archive of the discontinued LLVM Phabricator instance.

uint32_t options -> File::OpenOptions options

Authored by lawrence_danna on Oct 10 2019, 7:42 PM.



This patch re-types everywhere that passes a File::OpenOptions
as a uint32_t so it actually uses File::OpenOptions.

It also converts some OpenOptions related functions that fail
by returning 0 or NULL into llvm::Expected

split off from

Diff Detail

Event Timeline

lawrence_danna created this revision.Oct 10 2019, 7:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2019, 7:42 PM

I originally thought you would do something local, but changing this across the board is definitely better, and it doesn't seem the patch is that big. LLVM already has a utility to work out the kinks in the bitmask enum use case. Please take a look at llvm/ADT/BitmaskEnum.h on how to make use of that.


A better way to handle that is to make this a typed enum (enum OpenOptions : uint16_t). Then you can add something like LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue=*/0xffffu) to make sure the bitmask operators also work well.


I'd consider leaving the static function as const char * -- the function doesn't do anything complex, and there's only one way it can fail (which can be signalled by a nullptr), so having the Expected wrapper around that does not seem all that useful -- I'll leave it up to you though.


For similar reasons, I'd make this an Optional<OpenOptions>.


If GetOptionsFromMode stays as Expected then this is not correct, as it does not handle the error case. Overall, I'd say we shouldn't use auto here..

lawrence_danna marked 5 inline comments as done.

review fixes


If I do that I'd still just have to convert it to Error in the callers, so I think I'll leave it this way.

labath accepted this revision.Oct 14 2019, 12:51 AM
labath added inline comments.

Yes, but OTOH, if you did that, there would also be callers where you wouldn't need the consumeError(.takeError()) stuff. Anyway, I think this is fine too...

This revision is now accepted and ready to land.Oct 14 2019, 12:51 AM
This revision was automatically updated to reflect the committed changes.