Page MenuHomePhabricator

Make the error condition in Value::ValueType explicit (NFC)
ClosedPublic

Authored by aprantl on Feb 11 2021, 12:47 PM.

Details

Summary

The comment for ValueType claims that all values <1 are errors, but not all switch statements take this into account. This patch introduces an explicit Error case and deletes all default: cases, so we get warned about incomplete switch coverage.

Diff Detail

Event Timeline

aprantl created this revision.Feb 11 2021, 12:47 PM
aprantl requested review of this revision.Feb 11 2021, 12:47 PM
aprantl updated this revision to Diff 323119.Feb 11 2021, 12:49 PM

Phabricator got really confused by the diff context. Reposting without context.

aprantl updated this revision to Diff 323120.Feb 11 2021, 12:55 PM
aprantl updated this revision to Diff 323135.Feb 11 2021, 1:42 PM

Found two more uncovered cases. Drive-by fix of two error messages.

This LGTM module the unrelated changes in that one file. A third pair of eyes probably won't hurt, so I'll give the others a change to look over this before I accept, but otherwise I'll just accept tomorrow.

(Btw, if you intend to cherry-pick this into swift/main for some reason then please let me know as this probably will cause a bunch of conflicts with the temporarily reverted patch from Pavel).

lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
38

The changes in this whole file seem like an accidental commit slipped in.

aprantl updated this revision to Diff 323176.Feb 11 2021, 3:42 PM

Upload the correct patch this time!? Hopefully?

shafik added inline comments.Feb 11 2021, 5:31 PM
lldb/source/Core/Value.cpp
321

Would it make sense to also do an error.SetErrorString(...?

575–576

In the invalid case does m_value have some initial value i.e. it is not uninitialized.

629

I love the "???"

lldb/source/Core/ValueObject.cpp
339–343

No handing for ValueType::Invalid here?

858–861

No handling for ValueType::Invalid here?

Same for some code below as well.

lldb/source/Core/ValueObjectConstResult.cpp
148

dead code?

lldb/source/Core/ValueObjectVariable.cpp
200

Does m_error.SetErrorString make sense here?

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp
1668

Dead code?

lldb/source/Target/ABI.cpp
122–123

No handling for ValueType::Invalid here?

aprantl updated this revision to Diff 323467.Feb 12 2021, 1:44 PM
aprantl marked 9 inline comments as done.

Address review feedback!

shafik accepted this revision.Feb 12 2021, 2:23 PM

LGTM

This revision is now accepted and ready to land.Feb 12 2021, 2:23 PM

(Btw, if you intend to cherry-pick this into swift/main for some reason then please let me know as this probably will cause a bunch of conflicts with the temporarily reverted patch from Pavel).

I don't need to cherry-pick it, so I won't for now.

lldb/source/Core/Value.cpp
575–576

I don't understand what you are asking here?

lldb/source/Core/ValueObjectConstResult.cpp
148

Yes, but I'm not going to touch this in this commit.

shafik added inline comments.Feb 12 2021, 10:23 PM
lldb/source/Core/Value.cpp
575–576

The other cases initialize m_value or call Clear on it and m_value is return at the end and I was asking is it is properly initialized.

aprantl closed this revision.Feb 17 2021, 9:22 AM

This landed as 057efa9916cdc354ef4653bcd128a578cc43125e I messed up the commit message.