[Lldb-commits] [PATCH] D25196: Adding a new Minidump post-mortem debugging plugin

[Zachary Turner via lldb-commits]

zturner added inline comments.


> MinidumpParser.cpp:81-82
>  
> -  return llvm::ArrayRef<uint8_t>(m_data_sp->GetBytes() + iter->second.rva,
> +  return llvm::ArrayRef<uint8_t>(GetData().data() + iter->second.rva,
>                                   iter->second.data_size);
>  }

Change this line to  `return GetData().slice(iter->second.rva, iter->second.data_size);`

> MinidumpParser.cpp:106-107
> +    return llvm::None;
> +  return {GetData().data() + td.thread_context.rva,
> +          td.thread_context.data_size};
> +}

Same as above, use `slice()`

> MinidumpParser.cpp:255
> +          range_start,
> +          llvm::ArrayRef<uint8_t>(GetData().data() + loc_desc.rva, range_size));
> +    }

`slice()`

> MinidumpParser.h:35-36
> +struct Range {
> +  lldb::addr_t start; // virtual address of the beginning of the range
> +  // absolute pointer to the first byte of the range and size
> +  llvm::ArrayRef<uint8_t> range_ref;

If the comment is long enough to wrap, maybe better to just put it on the line before.  Looks awkward this way.

> MinidumpParser.h:39-40
> +
> +  Range(lldb::addr_t start_, llvm::ArrayRef<uint8_t> range_ref_)
> +      : start(start_), range_ref(range_ref_) {}
> +};

You don't need the underscores here.  It might look awkward, but the usual LLVM pattern is to just call the constructor parameters and member variables the same name.

> MinidumpTypes.cpp:188-190
> +  return llvm::ArrayRef<MinidumpMemoryDescriptor>(
> +      reinterpret_cast<const MinidumpMemoryDescriptor *>(data.data()),
> +      *mem_ranges_count);

you can write `return llvm::makeArrayRef(reinterpret_cast<const MinidumpMemoryDescriptor*>(data.data()), *mem_ranges_count));` to avoid specifying the type name twice.  It's a little shorter (admittedly not much though).

https://reviews.llvm.org/D25196