[Lldb-commits] [PATCH] D158041: [lldb] [gdb-remote] Also handle high/low memory addressable bits setting in the stop info packet

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added a comment.

In D158041#4593205 <https://reviews.llvm.org/D158041#4593205>, @jasonmolenda wrote:

> Update patch to change the `AddressableBits::IsValid` to `AddressableBits::HasValue` which is a clearer description.  The `IsValid` methods are used across the lldb_private classes everywhere.  I tried changing this to `operator bool` but I thought that was less clear in use.  I compromised by changing it to `HasValue` - this is a POD class so "IsValid" seemed a little questionable, but I didn't like operator bool.  Let's try this out.

Yeah that's fine. I don't feel strongly about `operator bool()`. My main concern is to not create another class that can be in an "invalid state" and requires you to check whether or not it's valid before doing an operation. As you point out, that's not really the case here anyway, and using different terminology helps convey that.



================
Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:2314-2316
+    if (addressable_bits.HasValue()) {
+      addressable_bits.SetProcessMasks(*this);
+    }
----------------
I would inline the check for `HasValue` in `SetProcessMasks`. You already do some kind of sanity checking there, might as well make that sound and do it unconditionally. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158041/new/

https://reviews.llvm.org/D158041