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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
