[PATCH] D50269: [compiler-rt][builtins] Don't #include CoreFoundation in os_version_check.c

Duncan P. N. Exon Smith via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Oct 29 14:16:41 PDT 2018


dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

This LGTM with a couple of nitpicks.



================
Comment at: compiler-rt/lib/builtins/os_version_check.c:183
   CFStringRef ProductVersion = (*CFStringCreateWithCStringNoCopyFunc)(
-      NULL, "ProductVersion", kCFStringEncodingASCII, kCFAllocatorNull);
+      NULL, "ProductVersion", CF_STRING_ENCODING_ASCII, kCFAllocatorNull);
   if (!ProductVersion)
----------------
It wasn't obvious that `kCFAllocatorNull` had been `dlsym`'ed in.  Is there a way of naming this that would make it more clear you weren't getting this from an `#include`?  Or does it not matter?


================
Comment at: compiler-rt/lib/builtins/os_version_check.c:211-214
+  if (Major < GlobalMajor) return 1;
+  if (Major > GlobalMajor) return 0;
+  if (Minor < GlobalMinor) return 1;
+  if (Minor > GlobalMinor) return 0;
----------------
If you're going to do this, please do it in a separate NFC commit since it appears to be whitespace only.


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D50269





More information about the llvm-commits mailing list