[Lldb-commits] [PATCH] D68853: uint32_t options -> File::OpenOptions options

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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.



================
Comment at: lldb/include/lldb/Host/File.h:56
+        (1u << 8),               // Close the file when executing a new process
+    eOpenOptionMax = 0xffffffffu // avoid undefined behavior
   };
----------------
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.



================
Comment at: lldb/source/Host/common/File.cpp:42
 
-static const char *GetStreamOpenModeFromOptions(uint32_t options) {
+static Expected<const char *> GetStreamOpenModeFromOptions(uint32_t options) {
   if (options & File::eOpenOptionAppend) {
----------------
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.


================
Comment at: lldb/source/Host/common/File.cpp:74
+
+Expected<File::OpenOptions> File::GetOptionsFromMode(llvm::StringRef mode) {
+  OpenOptions opts =
----------------
For similar reasons, I'd make this an Optional<OpenOptions>.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1093
   auto options = File::GetOptionsFromMode(py_mode.GetString());
-  auto file = std::unique_ptr<File>(
-      new NativeFile(PyObject_AsFileDescriptor(m_py_obj), options, false));
+  if (!options)
+    return nullptr;
----------------
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..


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68853/new/

https://reviews.llvm.org/D68853