jasonmolenda accepted this revision.
jasonmolenda added a comment.
This revision is now accepted and ready to land.
looks good to me.
================
Comment at: tools/debugserver/source/MacOSX/x86_64/DNBArchImplX86_64.cpp:66
+ int error = ::sysctlbyname(feature, &answer, &answer_size, NULL, 0);
+ return !error & answer;
+}
----------------
zturner wrote:
> jasonmolenda wrote:
> > I see what you're doing -- this can either return "false, meaning the
> > sysctlbyname failed or I got a zero value, or true meaning it succeeded and
> > I got a nonzero value" but the use of a bitwise AND is going to look like a
> > bug to any casual reader and make them re-read the code a few times before
> > they've realized it is intended (at least it did me). It might be easier
> > for future maintainers if it's written more like like
> >
> > if (error != 0 && answer != 0)
> > return true;
> > else
> > return false;
> >
> > and let the compiler come up with the shorter form when generating the
> > instructions.
> Maybe I'm missing something, but why not just `return !error && answer`?
That would be fine too. The bitwise in the original is correct, but needlessly
confusing IMO.
================
Comment at: tools/debugserver/source/MacOSX/x86_64/DNBArchImplX86_64.cpp:99
+ for (; first_letter < length; ++first_letter) {
+ if (buffer[first_letter] & 0x40)
+ break;
----------------
jasonmolenda wrote:
> Would '@' be clearer here than 0x40? I had to run man ascii to figure out
> what this was. Maybe a comment saying that we expect the string from
> KERN_OSVERSION to be something like '11B156' & we're interested in the '11'
> bit would make it a little easier to read. (I had to run sysctl to look at
> the string you're parsing to follow along with the logic)
I misread this myself when writing the above. isupper() is what you're doing -
I'd recommend using that instead of the bit mask.
https://reviews.llvm.org/D30918
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
