[PATCH] D40244: [dsymutil] Upstream getBundleInfo implementation

[Adrian Prantl via Phabricator via llvm-commits]

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

Couple of stylistic comments, otherwise this looks good.



================
Comment at: tools/dsymutil/CFBundle.cpp:54
+
+class CFBundle : public CFReleaser<CFBundleRef> {
+public:
----------------
comment what the class is for?


================
Comment at: tools/dsymutil/CFBundle.cpp:64
+
+static const char *CreatePathByExpandingTildePath(const char *Path,
+                                                  std::string &ExpandedPath) {
----------------
comment what this function is doing?


================
Comment at: tools/dsymutil/CFBundle.cpp:76
+
+CFString::CFString(CFStringRef s) : CFReleaser<CFStringRef>(s) {}
+
----------------
It might make sense to move all these short and undocumented functions into the class declaration.


================
Comment at: tools/dsymutil/CFBundle.cpp:86
+
+const char *CFString::GetFileSystemRepresentation(std::string &Str) {
+  return CFString::FileSystemRepresentation(get(), Str);
----------------
move into declaration?


================
Comment at: tools/dsymutil/CFBundle.cpp:99
+
+CFStringRef CFString::SetFileSystemRepresentationFromCFType(CFTypeRef CFType) {
+  CFStringRef NewValue = nullptr;
----------------
comment what this function is doing?


================
Comment at: tools/dsymutil/CFBundle.cpp:182
+
+CFIndex CFString::GetLength() const {
+  if (CFStringRef Str = get())
----------------
perhaps move into declaration?


================
Comment at: tools/dsymutil/CFBundle.cpp:188
+
+CFBundle::CFBundle(const char *Path) : CFReleaser<CFBundleRef>() {
+  if (Path && Path[0])
----------------
 move into declaration?


================
Comment at: tools/dsymutil/CFBundle.cpp:193
+
+CFBundle::CFBundle(CFURLRef url)
+    : CFReleaser<CFBundleRef>(url ? CFBundleCreate(nullptr, url) : nullptr) {}
----------------
move into declaration?


================
Comment at: tools/dsymutil/CFBundle.cpp:238
+
+CFStringRef CFBundle::GetIdentifier() const {
+  if (CFBundleRef bundle = get())
----------------
move into declaration?


================
Comment at: tools/dsymutil/CFBundle.cpp:244
+
+CFTypeRef CFBundle::GetValueForInfoDictionaryKey(CFStringRef key) const {
+  if (CFBundleRef bundle = get())
----------------
move into declaration?


================
Comment at: tools/dsymutil/CFBundle.cpp:266
+
+  // Try and find the original executable's Info.plist information using
+  // CoreFoundation calls by creating a URL for the executable and chopping off
----------------
should this be the doxygen comment for the function?


================
Comment at: tools/dsymutil/CFBundle.h:5
+
+void getBundleInfo(llvm::StringRef ExePath, std::string &bundleVersionStr,
+                   std::string &bundleShortVersionStr, std::string &bundleIDStr,
----------------
JDevlieghere wrote:
> aprantl wrote:
> > does this only work when `__APPLE__` is set? Otherwise, this would be a better fit for Support since it might be useful for other tools, too.
> On non `__APPLE__` the function returns the default ("1" and "1.0"). The advantage of keeping this here is that we only have to link dsymutil against CoreFoundation. Do you think it's worth the trade-off? 
The extra dependency is a pretty strong argument for keeping it in dsymutil, thanks!


Repository:
  rL LLVM

https://reviews.llvm.org/D40244