jasonmolenda requested changes to this revision.
jasonmolenda added a comment.
This revision now requires changes to proceed.
Looks good, a few small suggestions from reading the code afresh. I'm not sure
how many open source contributors we may have running macOS 10.6 or earlier (or
macOS 10.7.[1-3]) but keeping this code is probably still a good idea at this
point.
================
Comment at: tools/debugserver/source/MacOSX/x86_64/DNBArchImplX86_64.cpp:66
+ int error = ::sysctlbyname(feature, &answer, &answer_size, NULL, 0);
+ return !error & answer;
+}
----------------
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.
================
Comment at: tools/debugserver/source/MacOSX/x86_64/DNBArchImplX86_64.cpp:99
+ for (; first_letter < length; ++first_letter) {
+ if (buffer[first_letter] & 0x40)
+ break;
----------------
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)
================
Comment at: tools/debugserver/source/MacOSX/x86_64/DNBArchImplX86_64.cpp:103
+ char letter = buffer[first_letter];
+ buffer[first_letter] = 0;
+ auto major_ver = strtoull(buffer, NULL, 0);
----------------
Not a big deal but '\0'.
https://reviews.llvm.org/D30918
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
