[Lldb-commits] [PATCH] D96537: Make the error condition in Value::ValueType explicit (NFC)

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added inline comments.


================
Comment at: lldb/source/Core/Value.cpp:321
+  case ValueType::Invalid:
+    break;
+  case ValueType::Scalar: {
----------------
Would it make sense to also do an `error.SetErrorString(...`? 


================
Comment at: lldb/source/Core/Value.cpp:574
     switch (m_value_type) {
-    case eValueTypeScalar: // raw scalar value
+    case ValueType::Invalid:
+    case ValueType::Scalar: // raw scalar value
----------------
In the invalid case does `m_value` have some initial value i.e. it is not uninitialized. 


================
Comment at: lldb/source/Core/Value.cpp:629
   };
-  return "???";
 }
----------------
I love the `"???"`


================
Comment at: lldb/source/Core/ValueObject.cpp:339
       switch (value_type) {
-      case Value::eValueTypeScalar:
-        if (value.GetContextType() == Value::eContextTypeRegisterInfo) {
+      case Value::ValueType::Scalar:
+        if (value.GetContextType() == Value::ContextType::RegisterInfo) {
----------------
No handing for `ValueType::Invalid` here?


================
Comment at: lldb/source/Core/ValueObject.cpp:855
   switch (value_type) {
-  case Value::eValueTypeScalar: {
+  case Value::ValueType::Scalar: {
     Status set_error =
----------------
No handling for `ValueType::Invalid` here?

Same for some code below as well.


================
Comment at: lldb/source/Core/ValueObjectConstResult.cpp:148
   m_value.GetScalar().GetData(m_data, addr_byte_size);
-  // m_value.SetValueType(Value::eValueTypeHostAddress);
+  // m_value.SetValueType(Value::ValueType::HostAddress);
   switch (address_type) {
----------------
dead code?


================
Comment at: lldb/source/Core/ValueObjectVariable.cpp:200
+      case Value::ValueType::Invalid:
+        break;
+      case Value::ValueType::Scalar:
----------------
Does `m_error.SetErrorString` make sense here?


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:1668
 
-  // parser_vars->m_lldb_value.SetContext(Value::eContextTypeClangType,
+  // parser_vars->m_lldb_value.SetContext(Value::ContextType::ClangType,
   // user_type.GetOpaqueQualType());
----------------
Dead code?


================
Comment at: lldb/source/Target/ABI.cpp:123
     switch (result_value.GetValueType()) {
-    case Value::eValueTypeHostAddress:
-    case Value::eValueTypeFileAddress:
+    case Value::ValueType::HostAddress:
+    case Value::ValueType::FileAddress:
----------------
No handling for `ValueType::Invalid` here?


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

https://reviews.llvm.org/D96537