Replace std::unique_ptr<T> with HeapArray [crashpad/crashpad : main]
Mark Mentovai (Gerrit)
Mark Mentovai added 6 comments![]( "Open in Gerrit")
result.assign(buf.data(), size);Why do we want a `base::HeapArray<char>` if we’re just going to copy it to a `std::string`? Why not start out with a 4kB string and resize it down? The only practical difference is that we can’t have `Uninit` semantics on the string.
#include <memory>You can get rid of this now. Did you check other files to see where else you can get rid of `<memory>`?
if (!process_memory_->Read(address_, size_, buffer.data())) {I’d use `buffer.size()` here because it’s more direct (“fill `buffer` up to its size”).
auto argv_as_utf8 = base::HeapArray<char*>::Uninit(argc + 1);We never consider the size of `argv_as_utf8`, so I’m curious why this one would need to change.
EXPECT_TRUE(memory.Read(page_addr1, result.size() / 2, result.data()));These divisions are quirky, I’d have stuck with `baes::GetPageSize()`.
Related details
- Arthur Wang
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Wang (Gerrit)
Arthur Wang voted and added 6 comments![]( "Open in Gerrit")
Votes added by Arthur Wang
| Commit-Queue | +1 |
6 comments
Why do we want a `base::HeapArray<char>` if we’re just going to copy it to a `std::string`? Why not start out with a 4kB string and resize it down? The only practical difference is that we can’t have `Uninit` semantics on the string.
Done
You can get rid of this now. Did you check other files to see where else you can get rid of `<memory>`?
Done
if (!process_memory_->Read(address_, size_, buffer.data())) {I’d use `buffer.size()` here because it’s more direct (“fill `buffer` up to its size”).
Done
auto argv_as_utf8 = base::HeapArray<char*>::Uninit(argc + 1);We never consider the size of `argv_as_utf8`, so I’m curious why this one would need to change.
This is just for consistency reason as the tool just blindly file bugs based on regex match. so if we keep this unchanged, the bug will have to be kept alive.
EXPECT_TRUE(memory.Read(page_addr1, result.size() / 2, result.data()));These divisions are quirky, I’d have stuck with `baes::GetPageSize()`.
- Mark Mentovai
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
buf.reserve(kMaxSize);This is invalid. You need to `resize`. More importantly, the whole point of this was that you could do it all in `result` without needing `buf` at all.
Related details
- Arthur Wang
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Wang (Gerrit)
Arthur Wang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Arthur Wang
| Commit-Queue | +1 |
1 comment
This is invalid. You need to `resize`. More importantly, the whole point of this was that you could do it all in `result` without needing `buf` at all.
- Mark Mentovai
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
buf.reserve(kMaxSize);Arthur WangThis is invalid. You need to `resize`. More importantly, the whole point of this was that you could do it all in `result` without needing `buf` at all.
Acknowledged
- Arthur Wang
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 10 comments![]( "Open in Gerrit")
std::string GetProcessNameFromPid(pid_t pid);You need to `#include <sys/types.h>` or another header documented as providing `pid_t`.
This file actually uses `pid_t` already, so you should make this change even if you don’t wind up declaring this function in this header.
std::string GetProcessNameFromPid(pid_t pid);If this is here, it needs to be documented.
You put this here to be able to test it, but I don’t think that the test is very strong, so I wouldn’t mind if you just took this (and the new test) out and put the function back in the unnamed namespace.
// Returns the process name for a pid.If you are documenting this function in the header, it should be removed from here.
PLOG(ERROR) << "Failed to readlink " << link_path;`result.clear();`
See comments elsewhere: this test doesn’t really do that much, so I’d be just as happy if you didn’t include it.
namespace crashpad {
namespace test {Crashpad puts tests inside an unnamed namespace inside `crashpad::test`. Look around, you’ll find plenty of examples.
TEST(CrosCrashReportExceptionHandler, GetProcessNameFromPidTest) {No `Test` suffix necessary.
std::string result = GetProcessNameFromPid(getppid());`#include <string>`
std::string result = GetProcessNameFromPid(getppid());`#include <unistd.h>`
ASSERT_GT(result.size(), size_t{0});`ASSERT_FALSE(result.empty());` is much more direct than this.
Related details
- Arthur Wang
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Wang (Gerrit)
Arthur Wang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Arthur Wang
| Commit-Queue | +1 |
1 comment
buf.reserve(kMaxSize);Arthur WangThis is invalid. You need to `resize`. More importantly, the whole point of this was that you could do it all in `result` without needing `buf` at all.
Mark MentovaiAcknowledged
Acknowledged
You acknowledged but did not fully fix.
- Arthur Wang
- Mark Mentovai
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Wang (Gerrit)
Arthur Wang added 10 comments![]( "Open in Gerrit")
If this is here, it needs to be documented.
You put this here to be able to test it, but I don’t think that the test is very strong, so I wouldn’t mind if you just took this (and the new test) out and put the function back in the unnamed namespace.
Acknowledged
You need to `#include <sys/types.h>` or another header documented as providing `pid_t`.
This file actually uses `pid_t` already, so you should make this change even if you don’t wind up declaring this function in this header.
Done
If you are documenting this function in the header, it should be removed from here.
Acknowledged
PLOG(ERROR) << "Failed to readlink " << link_path;Arthur Wang`result.clear();`
Done
See comments elsewhere: this test doesn’t really do that much, so I’d be just as happy if you didn’t include it.
Acknowledged
Crashpad puts tests inside an unnamed namespace inside `crashpad::test`. Look around, you’ll find plenty of examples.
Acknowledged. n/a as I am removing this unittest file.
TEST(CrosCrashReportExceptionHandler, GetProcessNameFromPidTest) {Arthur WangNo `Test` suffix necessary.
Acknowledged. n/a as I am removing this unittest file.
std::string result = GetProcessNameFromPid(getppid());Arthur Wang`#include <string>`
Acknowledged. n/a as I am removing this unittest file.
std::string result = GetProcessNameFromPid(getppid());Arthur Wang`#include <unistd.h>`
Acknowledged. n/a as I am removing this unittest file.
`ASSERT_FALSE(result.empty());` is much more direct than this.
- Mark Mentovai
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
#include <map>Blank line between C and C++ system headers.
Related details
- Arthur Wang
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Wang (Gerrit)
Arthur Wang added 1 comment![]( "Open in Gerrit")
Blank line between C and C++ system headers.
- Mark Mentovai
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai voted Code-Review+1![]( "Open in Gerrit")
- Arthur Wang
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Wang (Gerrit)
Arthur Wang voted Commit-Queue+2![]( "Open in Gerrit")
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Crashpad LUCI CQ (Gerrit)
Crashpad LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Replace std::unique_ptr<T> with HeapArray
- M handler/linux/cros_crash_report_exception_handler.cc
- M handler/linux/cros_crash_report_exception_handler.h
- M snapshot/capture_memory.cc
- M snapshot/memory_snapshot_generic.h
- M snapshot/win/pe_image_reader.cc
- M tools/tool_support.cc
- M util/net/http_body_gzip_test.cc
- M util/net/http_body_test_util.cc
- M util/process/process_memory_mac_test.cc
- M util/process/process_memory_test.cc
- M util/win/exception_handler_server.cc
- M util/win/module_version.cc
Code-Review: +1 by Mark Mentovai
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |