This is an archive of the discontinued LLVM Phabricator instance.

Add failure paths to a few JSONNumber members
ClosedPublic

Authored by omjavaid on Dec 8 2015, 3:05 PM.

Details

Summary

This patch updates GetAsUnsigned(), GetAsSigned(), and GetAsDouble() JSONNumber functions to add failure check.

The previous code was generating compiler warnings for not being able to provide a return path for all possibilities.

Diff Detail

Event Timeline

omjavaid updated this revision to Diff 42233.Dec 8 2015, 3:05 PM
omjavaid retitled this revision from to Add failure paths to a few JSONNumber members.
omjavaid updated this object.
omjavaid added a reviewer: tberghammer.
omjavaid added a subscriber: lldb-commits.
tberghammer requested changes to this revision.Dec 9 2015, 2:41 AM
tberghammer edited edge metadata.

These GetAs{...} functions should never fail in their current implementation as we have only 3 different data type and all of them are handled. The new function signatures you proposed are making the API significantly harder to use with no benefit. (Also they will produce clang warnings because of the default in a switch covering all cases.)

To silent the warning I would suggest to do this:

double
JSONNumber::GetAsDouble() const
{
    switch (m_data_type)
    {
        case DataType::Unsigned:
          return (double)m_data.m_unsigned;
        case DataType::Signed:
          return (double)m_data.m_signed;
        case DataType::Double:
          return m_data.m_double;
    }
    assert(false && "Unhandled data type");
    return 0;
}

In this code:

  • Gcc won't emit warnings because all code path have a return value.
  • Clang will emit a warning if somebody adds a new data type without handling them in these GetAs{...} functions (this is why I don't want a default case).
  • We have an additional runtime check for the fail case with the assert if the warnings are ignored.
This revision now requires changes to proceed.Dec 9 2015, 2:41 AM
labath added a subscriber: labath.Dec 9 2015, 2:50 AM

btw, if you use llvm_unreachable("text"), then you don't need the return after that.

omjavaid abandoned this revision.Dec 14 2015, 1:57 AM

Correction made upstream by @tberghammer.

I don't know what correction you are referring to as I haven't made any change in JSON.{h,cpp} since you created this patch but I am happy with the current situation (no warning on clang)

omjavaid updated this revision to Diff 42706.Dec 14 2015, 5:27 AM
omjavaid edited edge metadata.

Sorry @tberghammer there was corruption in my setup showing changes which i just made internally.

I have updated this diff as per suggestions.

LGTM?

tberghammer accepted this revision.Dec 14 2015, 5:31 AM
tberghammer edited edge metadata.

LGTM

include/lldb/Utility/JSON.h
147–148

Nice catch!

This revision is now accepted and ready to land.Dec 14 2015, 5:31 AM
This revision was automatically updated to reflect the committed changes.