[PATCH] D31236: Refactor getHostCPUName to allow testing on non-native hardware.

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added inline comments.


================
Comment at: llvm/trunk/include/llvm/Support/Host.h:80-85
+  /// helper functions to extract HostCPUName from /proc/cpuinfo on linux.
+  namespace LinuxReadCpuInfo {
+  StringRef getHostCPUName_powerpc(const StringRef &ProcCpuinfoContent);
+  StringRef getHostCPUName_arm(const StringRef &ProcCpuinfoContent);
+  StringRef getHostCPUName_s390x(const StringRef &ProcCpuinfoContent);
+  }
----------------
chandlerc wrote:
> Capitalize 'helper' to make the comment prose.
> 
> Also, we more commonly use a 'detail' or 'internal' namespace. That would seem more consistent here.
> 
> And please follow the normal naming conventions rather than using a '_<arch>' suffix. Perhaps: `getHostCPUNameForPowerPC`, `getHostCPUNameForARM`, and `getHostCPUNameForS390x`.
Thanks for the feedback!
I tried to fix those issues in r299062, which resulted in the windows builds breaking, due to error messages like the following:

```
C:\Buildbot\Slave\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\include\llvm/ADT/DenseSet.h(215): error C2872: 'detail': ambiguous symbol
C:\Buildbot\Slave\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\include\llvm/Support/Chrono.h(78): note: could be 'llvm::detail'
C:\Buildbot\Slave\llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast\llvm.src\include\llvm/Support/Host.h(80): note: or       'llvm::sys::detail'
```

A few attempts at fixing the Windows builds by fixing the issues reported in the buildbot logs showed that every fix just uncovered more "ambiguous symbol errors", so I reverted the changes.
I'll probably need to investigate if introducing an llvm::sys::detail namespace is still a good idea (which means the "detail::xxx" syntaxes in quite a few places will need to be disambiguated to "llvm::detail::xxx"); or if an alternative solution needs to be found.




Repository:
  rL LLVM

https://reviews.llvm.org/D31236