labath added inline comments.
================
Comment at: lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp:852
+ RegisterContextWindows *reg_ctx = static_cast<RegisterContextWindows *>(
+ thread->GetRegisterContext().get());
+ if (!reg_ctx->AddHardwareBreakpoint(info.slot, info.address, info.size,
----------------
aleksandr.urakov wrote:
> amccarth wrote:
> > Since you have to explicitly downcast, wouldn't `auto *reg_ctx` on the left
> > be sufficient and just as clear?
> Actually, I'm a big fan of auto, and some time ago @zturner have told me that
> normally it is not used so much in LLVM code, so I have reduced its usage in
> my patches :) But if it is OK to use auto here, I'll fix it (and in similar
> places too), thanks!
Yeah, llvm does not use auto too match, but in this case the type is already
explicitly present, so there's no ambiguity, and this is fine. (I would still
use `auto *` instead of plain `auto` though..)
================
Comment at:
lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp:82
-bool RegisterContextWindows::ClearHardwareBreakpoint(uint32_t hw_idx) {
- return false;
-}
+ if (!size || size > 8 || size & (size - 1))
+ return false;
----------------
aleksandr.urakov wrote:
> zturner wrote:
> > amccarth wrote:
> > > Clever! It took me a minute or two to figure out what the point of that
> > > was checking. Perhaps a comment to explain?
> > Isn't this equivalent to:
> >
> > ```
> > switch (size)
> > {
> > case 1:
> > case 2:
> > case 4:
> > case 8:
> > break;
> > default:
> > return false;
> > }
> > ```
> >
> > ? That definitely seems much clearer.
> >
> > I'm also pretty sure that on x86 you can't add a 64-bit watch, So you'd
> > have to do something different depending on the target bitness if you want
> > this to be correct for x86.
> Yes, it is equivalent, I've chosen the previous form due to its less
> verbosity. But you are right, clearance is better (especially after adding
> the architecture check). Fixed it, thanks!
.... or, you could just use `llvm::isPowerOf2_32` from `MathExtras.h`.
Repository:
rLLDB LLDB
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67168/new/
https://reviews.llvm.org/D67168
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
