This is an archive of the discontinued LLVM Phabricator instance.

[LLDB-MI] Minor cleanup of CMICmnLLDBUtilSBValue class
ClosedPublic

Authored by enlight on Oct 13 2016, 10:40 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

enlight updated this revision to Diff 74617.Oct 13 2016, 10:40 PM
enlight retitled this revision from to [LLDB-MI] Minor cleanup of CMICmnLLDBUtilSBValue class.
enlight updated this object.
enlight added reviewers: ki.stfu, abidh.
enlight set the repository for this revision to rL LLVM.
enlight added a subscriber: Restricted Project.
ki.stfu requested changes to this revision.Oct 13 2016, 10:54 PM
ki.stfu edited edge metadata.
ki.stfu added inline comments.
tools/lldb-mi/MICmnLLDBUtilSBValue.cpp
21–22 ↗(On Diff #74617)

use unnamed namespace here

22 ↗(On Diff #74617)

I dislike Placeholder suffix because the previous one is also a placeholder for unknown values. How about renaming to kUnresolvedValue/kUnresolvedCompositeValue?

This revision now requires changes to proceed.Oct 13 2016, 10:54 PM
enlight added inline comments.Oct 13 2016, 11:14 PM
tools/lldb-mi/MICmnLLDBUtilSBValue.cpp
21–22 ↗(On Diff #74617)

I would like to use an anonymous namespace but the LLVM coding standards say

make anonymous namespaces as small as possible, and only use them for class declarations

I don't really buy their reasoning, but we are supposed to be following their style now aren't we?

kUnresolvedValue/kUnresolvedCompositeValue does sound better, I'll rename them.

enlight updated this revision to Diff 74622.Oct 14 2016, 12:51 AM
enlight edited edge metadata.

Renamed placeholder c-strings.

ki.stfu accepted this revision.Oct 14 2016, 1:47 AM
ki.stfu edited edge metadata.
ki.stfu added inline comments.
tools/lldb-mi/MICmnLLDBUtilSBValue.cpp
21–22 ↗(On Diff #74617)

Okay, we have to follow the coding style.

22 ↗(On Diff #74617)

Actually I like kUnknownValue and I meant to rename the second one only. But it looks good to me also.

This revision is now accepted and ready to land.Oct 14 2016, 1:47 AM
ki.stfu requested changes to this revision.Oct 14 2016, 1:53 AM
ki.stfu edited edge metadata.
ki.stfu added inline comments.
tools/lldb-mi/MICmnLLDBUtilSBValue.cpp
116 ↗(On Diff #74622)

After couple of minutes I see the lack of logic there: the value is still composite but we return kUnresolvedValue instead of kUnresolvedCompositeValue.

I think we have to rename kUnresolvedValue back to kUnknownValue.

This revision now requires changes to proceed.Oct 14 2016, 1:53 AM
enlight updated this revision to Diff 74628.Oct 14 2016, 2:03 AM
enlight edited edge metadata.
enlight marked an inline comment as done.

Rename kUnresolvedValue back to kUknownValue.

enlight marked an inline comment as done.Oct 14 2016, 2:04 AM
enlight added inline comments.
tools/lldb-mi/MICmnLLDBUtilSBValue.cpp
116 ↗(On Diff #74622)

Yep, good point, I've renamed it back.

ki.stfu accepted this revision.Oct 14 2016, 2:19 AM
ki.stfu edited edge metadata.
This revision is now accepted and ready to land.Oct 14 2016, 2:19 AM
abidh accepted this revision.Oct 14 2016, 3:58 AM
abidh edited edge metadata.
This revision was automatically updated to reflect the committed changes.
enlight marked an inline comment as done.