[Lldb-commits] [PATCH] D56688: Make CompilerType::getBitSize() / getByteSize() return an optional result. (NFC)

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added a comment.

Sorry for the late review



================
Comment at: lldb/trunk/include/lldb/Target/ProcessStructReader.h:70
     }
-    size_t total_size = struct_type.GetByteSize(nullptr);
-    lldb::DataBufferSP buffer_sp(new DataBufferHeap(total_size, 0));
+    auto total_size = struct_type.GetByteSize(nullptr);
+    if (!total_size)
----------------
I will also second the sentiment on the `auto`, it does not improve readability.


================
Comment at: lldb/trunk/source/API/SBType.cpp:106
 uint64_t SBType::GetByteSize() {
-  if (!IsValid())
-    return 0;
-
-  return m_opaque_sp->GetCompilerType(false).GetByteSize(nullptr);
+  if (IsValid())
+    if (auto size = m_opaque_sp->GetCompilerType(false).GetByteSize(nullptr))
----------------
The early exit is more readable here and also has a lower mental load.


================
Comment at: lldb/trunk/source/Commands/CommandObjectMemory.cpp:647
+      if (!size) {
+        result.AppendError("can't get size of type");
+        return false;
----------------
This does not appear to be NFC


================
Comment at: lldb/trunk/source/Commands/CommandObjectMemory.cpp:1069
+        if (!size)
+          return false;
+        switch (*size) {
----------------
NFC?


================
Comment at: lldb/trunk/source/Core/Value.cpp:227
     if (ast_type.IsValid())
-      byte_size = ast_type.GetByteSize(
-          exe_ctx ? exe_ctx->GetBestExecutionContextScope() : nullptr);
+      if (auto size = ast_type.GetByteSize(
+              exe_ctx ? exe_ctx->GetBestExecutionContextScope() : nullptr))
----------------
This might read better as two steps, get the value and then check it.


================
Comment at: lldb/trunk/source/Expression/Materializer.cpp:50
+  if (auto size = type.GetByteSize(nullptr))
+    m_size = *size;
 
----------------
What value does `m_size` have otherwise?


================
Comment at: lldb/trunk/source/Expression/Materializer.cpp:800
+      if (!byte_size) {
+        err.SetErrorString("can't get size of type");
+        return;
----------------
NFC?


================
Comment at: lldb/trunk/source/Plugins/ABI/MacOSX-arm64/ABIMacOSX_arm64.cpp:2372
               lldb::offset_t offset = 0;
-              if (byte_size == sizeof(float)) {
+              if (*byte_size == sizeof(float)) {
                 value.GetScalar() = data.GetFloat(&offset);
----------------
switch?


================
Comment at: lldb/trunk/source/Plugins/ABI/MacOSX-i386/ABIMacOSX_i386.cpp:827
     CompilerType compiler_type(value->GetCompilerType());
-    if (compiler_type) {
+    auto bit_size = compiler_type.GetBitSize(&thread);
+    if (bit_size) {
----------------
Looks like you chopped out the `if (compiler_type) ` check


================
Comment at: lldb/trunk/source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp:1720
       if (homogeneous_count > 0 && homogeneous_count <= 4) {
+        auto base_byte_size = base_type.GetByteSize(nullptr);
         if (base_type.IsVectorType(nullptr, nullptr)) {
----------------
There are a lot of checks for `base_byte_size` so it is not clear what value `vfp_byte_size` will have if `base_byte_size` is empty.

The flow looks a lot different but hard to track what will be set and not set.


================
Comment at: lldb/trunk/source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp:1788
 
-    DataBufferSP data_sp(new DataBufferHeap(byte_size, 0));
+    DataBufferSP data_sp(new DataBufferHeap(*byte_size, 0));
     uint32_t data_offset = 0;
----------------
`make_shared`?


================
Comment at: lldb/trunk/source/Plugins/ABI/SysV-arm64/ABISysV_arm64.cpp:2343
               lldb::offset_t offset = 0;
-              if (byte_size == sizeof(float)) {
+              if (*byte_size == sizeof(float)) {
                 value.GetScalar() = data.GetFloat(&offset);
----------------
switch? 


================
Comment at: lldb/trunk/source/Plugins/ABI/SysV-s390x/ABISysV_s390x.cpp:379
     CompilerType compiler_type = value->GetCompilerType();
-    if (!compiler_type)
+    auto bit_size = compiler_type.GetBitSize(&thread);
+    if (!bit_size)
----------------
Looks the `if (!compiler_type)` was removed.


================
Comment at: lldb/trunk/source/Plugins/Language/CPlusPlus/LibCxx.cpp:228
       }
-      CompilerType pair_type(__i_->GetCompilerType().GetTypeTemplateArgument(0));
-      std::string name; uint64_t bit_offset_ptr; uint32_t bitfield_bit_size_ptr; bool is_bitfield_ptr;
-      pair_type = pair_type.GetFieldAtIndex(0, name, &bit_offset_ptr, &bitfield_bit_size_ptr, &is_bitfield_ptr);
+      CompilerType pair_type(
+          __i_->GetCompilerType().GetTypeTemplateArgument(0));
----------------
Unrelated formatting changes?


================
Comment at: lldb/trunk/source/Plugins/Language/CPlusPlus/LibCxx.cpp:243
       m_pair_ptr = nullptr;
-      if (addr && addr!=LLDB_INVALID_ADDRESS) {
-        ClangASTContext *ast_ctx = llvm::dyn_cast_or_null<ClangASTContext>(pair_type.GetTypeSystem());
+      if (addr && addr != LLDB_INVALID_ADDRESS) {
+        ClangASTContext *ast_ctx =
----------------
Unrelated formatting changes?


================
Comment at: lldb/trunk/source/Plugins/Language/CPlusPlus/LibCxx.cpp:268
           return false;
-        DataExtractor extractor(buffer_sp, process_sp->GetByteOrder(), process_sp->GetAddressByteSize());
-        auto pair_sp = CreateValueObjectFromData("pair", extractor, valobj_sp->GetExecutionContextRef(), tree_node_type);
+        DataExtractor extractor(buffer_sp, process_sp->GetByteOrder(),
+                                process_sp->GetAddressByteSize());
----------------
Formatting?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56688