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

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

I had to stop commenting because there's too many occurrences, but please remove `auto` pretty much throughout the patch.  A reader of the code will not necessarily be aware or remember that the function returns an `Optional`, and they will read the code as "if the size is 0, report an error".  So I think the type should be spelled explicitly in all cases.



================
Comment at: 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 think we shouldn't use `auto` here, although this is a minor nit and I don't feel strongly.


================
Comment at: include/lldb/Target/ProcessStructReader.h:73
+      return;
+    lldb::DataBufferSP buffer_sp(new DataBufferHeap(*total_size, 0));
     Status error;
----------------
Can we use `make_shared` here?


================
Comment at: source/API/SBType.cpp:107-108
+  if (IsValid())
+    if (auto size = m_opaque_sp->GetCompilerType(false).GetByteSize(nullptr))
+      return *size;
+  return 0;
----------------
`TypeImpl::GetCompilerType` needs to return an `Optional` also, otherwise you will have the same issue here with zero-size types.


================
Comment at: source/Commands/CommandObjectMemory.cpp:527-528
 
-      m_format_options.GetByteSizeValue() = clang_ast_type.GetByteSize(nullptr);
-
-      if (m_format_options.GetByteSizeValue() == 0) {
+      auto size = clang_ast_type.GetByteSize(nullptr);
+      if (!size) {
         result.AppendErrorWithFormat(
----------------
Can you remove `auto` here?  It's definitely not obvious that it's returning an `Optional` here, so I originally read this as "if the size is 0, return an error".


================
Comment at: source/Commands/CommandObjectMemory.cpp:650
 
-      bytes_read = clang_ast_type.GetByteSize(nullptr) *
-                   m_format_options.GetCountValue().GetCurrentValue();
+      auto size = clang_ast_type.GetByteSize(nullptr);
+      if (!size) {
----------------
Same, please remove `auto`.


================
Comment at: source/Commands/CommandObjectMemory.cpp:1072
         uint64_t value = result_sp->GetValueAsUnsigned(0);
-        switch (result_sp->GetCompilerType().GetByteSize(nullptr)) {
+        auto size = result_sp->GetCompilerType().GetByteSize(nullptr);
+        if (!size)
----------------
`auto`


================
Comment at: source/Core/Value.cpp:227-229
+      if (auto size = ast_type.GetByteSize(
+              exe_ctx ? exe_ctx->GetBestExecutionContextScope() : nullptr))
+        byte_size = *size;
----------------
If the ast type is valid (if-check just above), then is it actually possible for this method to fail?


================
Comment at: source/Core/Value.cpp:349-351
+      if (auto size = ast_type.GetByteSize(
+              exe_ctx ? exe_ctx->GetBestExecutionContextScope() : nullptr))
+        limit_byte_size = *size;
----------------
Same here.  Can this actually fail?


================
Comment at: source/Core/ValueObject.cpp:759-762
+  auto item_type_size = pointee_or_element_compiler_type.GetByteSize(
       exe_ctx.GetBestExecutionContextScope());
-  const uint64_t bytes = item_count * item_type_size;
-  const uint64_t offset = item_idx * item_type_size;
+  if (!item_type_size)
+    return 0;
----------------
`auto`


================
Comment at: source/Core/ValueObject.cpp:827-828
     case eAddressTypeHost: {
-      const uint64_t max_bytes =
+      auto max_bytes =
           GetCompilerType().GetByteSize(exe_ctx.GetBestExecutionContextScope());
+      if (max_bytes && *max_bytes > offset) {
----------------
`auto`


================
Comment at: source/Core/ValueObject.cpp:1826-1828
+  auto size = type.GetByteSize(exe_ctx.GetBestExecutionContextScope());
+  if (!size)
+    return {};
----------------
`auto`


================
Comment at: source/Core/ValueObject.cpp:1829-1831
+  ValueObjectChild *synthetic_child =
+      new ValueObjectChild(*this, type, name_const_str, *size, offset, 0, 0,
+                           false, false, eAddressTypeInvalid, 0);
----------------
This should probably be `auto synthetic_child = std::make_shared<ValueObjectChild>(...);`


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56688