labath added a comment.
Yeah, this looks like it was fun to track down. I'm going to comment on both
patches here, since they are really similar...
Overall, I'm not very enthusiastic about the addr_t wrapper -- I'm not sure it
would be even possible because that's a part of the public API. It may be
possible to have something like lldb_private::addr_t, but I am not sure about
the usefulness of that either. Maybe it would make sense to create two helper
functions to convert pointers to and from lldb::addr_t "the right way".
As for these patches, instead of piling on casts, we should try to look for
other solutions where it makes sense. For instance, in many cases we have
parallel printf and formatv apis, and formatv usually does not require any
casts (vs. two casts with printf).
In other places you're replacing a reinterpret_cast<addr_t> with two c casts. I
guess this was done because two c++ casts were too long (?). In these cases the
second cast is not really needed, as uintptr_t->addr_t should convert
automatically. I think I'd prefer that we just have a single
reinterpret_cast<uintptr_t> instead of two c casts. Then our rule can be
"always convert a pointer to uintptr_t". I don't know if there's a clang-tidy
check for that, but it sounds like that could be something which could be
checked/enforced there...
================
Comment at: lldb/source/Expression/IRMemoryMap.cpp:577-586
if (lldb_private::Log *log =
lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS)) {
LLDB_LOGF(log,
"IRMemoryMap::WriteMemory (0x%" PRIx64 ", 0x%" PRIx64
", 0x%" PRId64 ") went to [0x%" PRIx64 "..0x%" PRIx64 ")",
- (uint64_t)process_address, (uint64_t)bytes, (uint64_t)size,
- (uint64_t)allocation.m_process_start,
+ (uint64_t)process_address, (uint64_t)(uintptr_t)bytes,
+ (uint64_t)size, (uint64_t)allocation.m_process_start,
----------------
This could be something
`LLDB_LOG(GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS), "({0:x}, {1:x},
{2:x}) went to [{3:x}, {4:x})", process_address, bytes,
allocation.m_process_start, allocation.m_process_start + allocation.m_size)`
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71498/new/
https://reviews.llvm.org/D71498
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
