switch on enum should be exhaustive and warning-free

[penryu]

Summary

Testing the value of type_code against the closed enum TypeCodes
provides statically verifiable completeness of testing. However, one
branch assigns to type_code by casting directly from a masked integer
value. This is currently handled by adding a default: case after
checking each TypeCodes instance. This patch introduces a bool variable
containing the "default" state value, allowing the switch to be
exhaustive, protect against future instances not being handled in the
switch, and preserves the original logic.

This addresses the warning:
warning: default label in switch which covers all enumeration values
[-Wcovered-switch-default]

As an issue of maintainability, the bitmask on line 524 handles the
current values of TypeCodes enum, but this will be invalid if the enum
is extended. This patch does not address this, and a more closed
conversion from cfinfoa -> TypeCodes would help protect against this.

[sas]

Looks like the numeric values of TypeCode are contiguous. It would probably be cleaner to a check after the cast to validate the value instead of having to add a line for each label in the switch statement. Something like this:

if (type_code < TypeCodes::sint8 || type_code > TypeCodes::sint128)
    return false;

You could also add TypeCodesBegin = -1 and TypeCodesEnd as the first and last enumerations of TypeCodes so the above check would not have to be modified when you change the definition of TypeCodes.

Other than that, looks good.

[penryu]

Thanks, sas.

I'll be honest, my prefer solution involves an inlined function (uint64_t -> TypeCodes) that eliminates the cast from the NSNumberSummaryProvider() method altogether. This way we can handle any dirty mappings from raw memory directory to the enum within the one function, and take advantage of the compiler's exhaustiveness check to ensure we're covering our bases.

I'll ponder a bit and consider reworking this patch in that form.

[sas]

Putting that check in an inline function instead of doing ad-hoc validation in the call-site seems better indeed.

[penryu]

I need to learn more about the number formatters before I make more significant changes than those below. As written currently, it leaves the logic intact and resolves the warning. One step closer to warning-free LLDB!

source/Plugins/Language/ObjC/Cocoa.cpp

if (descriptor->GetTaggedPointerInfo(&i_bits, &value)) {
           case 4: type_code = TypeCodes::sint64; break;
           case 5: type_code = TypeCodes::f32; break;
           case 6: type_code = TypeCodes::f64; break;
           default: return false;
         }
       }
       
       uint64_t value = 0;
+      bool success = false;
       switch (type_code) {
         case TypeCodes::sint8:
         value = process_sp->ReadUnsignedIntegerFromMemory(data_location, 1, 0,
                                                           error);
         if (error.Fail())
           return false;
         NSNumber_FormatChar(valobj, stream, (char)value, options.GetLanguage());
+        success = true;
         break;
         case TypeCodes::sint16:
         value = process_sp->ReadUnsignedIntegerFromMemory(data_location, 2, 0,
                                                           error);
         if (error.Fail())
           return false;
         NSNumber_FormatShort(valobj, stream, (short)value,
                              options.GetLanguage());
+        success = true;
         break;
       case TypeCodes::sint32:
         value = process_sp->ReadUnsignedIntegerFromMemory(data_location, 4, 0,
                                                           error);
         if (error.Fail())
           return false;
         NSNumber_FormatInt(valobj, stream, (int)value, options.GetLanguage());
+        success = true;
         break;
       case TypeCodes::sint64:
         value = process_sp->ReadUnsignedIntegerFromMemory(data_location, 8, 0,
                                                           error);
         if (error.Fail())
           return false;
         NSNumber_FormatLong(valobj, stream, value, options.GetLanguage());
+        success = true;
         break;
       case TypeCodes::f32:
       {
         uint32_t flt_as_int = process_sp->ReadUnsignedIntegerFromMemory(
             data_location, 4, 0, error);
         if (error.Fail())
           return false;
         float flt_value = 0.0f;
         memcpy(&flt_value, &flt_as_int, sizeof(flt_as_int));
         NSNumber_FormatFloat(valobj, stream, flt_value, options.GetLanguage());
+        success = true;
         break;
       }
       case TypeCodes::f64:
       {
         uint64_t dbl_as_lng = process_sp->ReadUnsignedIntegerFromMemory(
             data_location, 8, 0, error);
         if (error.Fail())
           return false;
         double dbl_value = 0.0;
         memcpy(&dbl_value, &dbl_as_lng, sizeof(dbl_as_lng));
         NSNumber_FormatDouble(valobj, stream, dbl_value, options.GetLanguage());
+        success = true;
         break;
       }
       case TypeCodes::sint128: // internally, this is the same
       {
         uint64_t words[2];
         words[1] = process_sp->ReadUnsignedIntegerFromMemory(
             data_location, 8, 0, error);
         if (error.Fail())
           return false;
         words[0] = process_sp->ReadUnsignedIntegerFromMemory(
             data_location + 8, 8, 0, error);
         if (error.Fail())
           return false;
         llvm::APInt i128_value(128, words);
         NSNumber_FormatInt128(valobj, stream, i128_value, options.GetLanguage());
+        success = true;
         break;
       }
-      default:
-        return false;
       }
-      return true;
+      return success;
     }
   }
 
   return false;
 }
 
 bool lldb_private::formatters::NSURLSummaryProvider(
     ValueObject &valobj, Stream &stream, const TypeSummaryOptions &options) {