[Lldb-commits] [clang] [lldb] [llvm] [llvm][TargetParser] Return optional from getHostCPUFeatures (PR #97824)

[David Spickett via lldb-commits]

================
@@ -1710,15 +1710,17 @@ VendorSignatures getVendorSignature(unsigned *MaxLeaf) {
 
 #if defined(__i386__) || defined(_M_IX86) || \
     defined(__x86_64__) || defined(_M_X64)
-bool sys::getHostCPUFeatures(StringMap<bool> &Features) {
+std::optional<StringMap<bool>> sys::getHostCPUFeatures() {
   unsigned EAX = 0, EBX = 0, ECX = 0, EDX = 0;
   unsigned MaxLevel;
 
   if (getX86CpuIDAndInfo(0, &MaxLevel, &EBX, &ECX, &EDX) || MaxLevel < 1)
-    return false;
+    return {};
 
   getX86CpuIDAndInfo(1, &EAX, &EBX, &ECX, &EDX);
 
+  StringMap<bool> Features;
----------------
DavidSpickett wrote:

The caller creating a blank map to pass to this function is the symptom of what I'm looking to fix here. The implementations are still going to create a map if they're going to return features.

The motivation for the change is to allow:
```
if (auto features = sys::getCPUHostFeatures())
```
Instead of:
```
map features;
if (sys::getCPUHostFeatures(map)
```
As the optional better describes the situation. You either get a feature map or you don't. Versus bool+mutable ref which could allow any combination of states. The docstring helps but might as well use the typesystem to say the same thing as well.

If the callers were building a map as they went, I'd understand the mutable ref parameter. Like:
```
map features
if (add_some_features(features))
  if (add_some_more_features(features))
    filter_features(features)
```
But all uses create an empty map specifically to pass to the function.

...which is to say yeah, I should include the motivation in the commit message, let me do that :)

https://github.com/llvm/llvm-project/pull/97824