labath added inline comments.
================
Comment at: lldb/source/Plugins/Language/ObjC/CFBasicHash.cpp:102-107
+ m_ht_64 = new __CFBasicHash<uint64_t>();
+ size_t copied = data_extractor.CopyData(0, size, m_ht_64);
+
+ if (copied != size) {
+ delete m_ht_64;
+ m_ht_64 = nullptr;
----------------
No manual memory management, please.
================
Comment at: lldb/source/Plugins/Language/ObjC/CFBasicHash.cpp:35-71
+
+ size = sizeof(__CFBasicHash<uint32_t>::RuntimeBase);
+ size += sizeof(__CFBasicHash<uint32_t>::Bits);
+
+ DataBufferHeap buffer(size, 0);
+ m_exe_ctx_ref.GetProcessSP()->ReadMemory(addr, buffer.GetBytes(), size,
+ error);
----------------
mib wrote:
> davide wrote:
> > These two pieces of code, `m_ptr_size == 4` and `m_ptr_size == 8` are
> > surprisingly similar. I'm really worried we might have a bug in one of them
> > and miss the other. Is there anything we can do to share the code? [e.g.
> > templatize].
> Indeed, they're very similar and I already tried using templates (and SFINAE)
> to make it more generic, however I couldn't achieve that.
>
> Since the remote architecture might be different from lldb's, we can't use
> macros to generate the underlying struct with the right size. So, I decided
> to template the structure used by CF, and have one of each architecture as a
> class attribute (look at CFBasicHash.h:114).
>
> Basically it's a tradeoff I chose voluntarily: I preferred having the
> CFBasicHash class handle the architecture to only expose one CFBasicHash
> object in the CFDictionary and CFSet data-formatter, rather than having two
> CFBasicHash objects templated for each ptr_size and have all the logic
> duplicated for each different architecture AND each data formatters.
>
> If you can see a better way to do it, please let me know :)
```
template<typename T> updateFor(std::unique_ptr<__CFBasicHash<T>> &m_ht, ...)
if (m_ptr_size == 4)
updateFor<uint32_t>(m_ht_4, ...);
else if (m_ptr_size == 8)
updateFor<uint64_t>(m_ht_8, ...)
```
?
Or the entire class could be a template, inheriting from a common
(non-templatized) interface...
================
Comment at: lldb/source/Plugins/Language/ObjC/NSDictionary.cpp:178-188
+private:
+ // Prime numbers. Values above 100 have been adjusted up so that the
+ // malloced block size will be just below a multiple of 512; values
+ // above 1200 have been adjusted up to just below a multiple of 4096.
+ constexpr static const uintptr_t NSDictionaryCapacities[] = {
+ 0, 3, 6, 11, 19, 32, 52,
+ 85, 118, 155, 237, 390, 672, 1065,
----------------
mib wrote:
> davide wrote:
> > Maybe a reference to the foundation header where these are defined, if
> > public.
> It is not in a public header, that's why I copied the explanation.
Are these actually used anywhere?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78396/new/
https://reviews.llvm.org/D78396
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
