Page MenuHomePhabricator

[lldb-mi] Simplify MICmnMIOutOfBandRecord implementation.
ClosedPublic

Authored by brucem on Aug 4 2015, 3:38 AM.

Details

Summary
  • Remove extraneous members that were just storing temporary values.
  • OutOfBand_e parameters don't need to be const as they are scalars.
  • Switch from a map with CMIUtilString values to using a mapping function. This uses a switch statement which will generate a warning if a new result class is added.
  • Make BuildAsyncRecord a static function rather than a private member function so that we can construct the result text correctly and avoid having extra stuff in the header.

Diff Detail

Repository
rL LLVM

Event Timeline

brucem updated this revision to Diff 31315.Aug 4 2015, 3:38 AM
brucem retitled this revision from to [lldb-mi] Simplify MICmnMIOutOfBandRecord implementation..
brucem updated this object.
brucem added reviewers: abidh, ki.stfu.
brucem added a subscriber: lldb-commits.
ki.stfu requested changes to this revision.Aug 4 2015, 5:17 AM
ki.stfu edited edge metadata.

I'd prefer to declare all static functions as private static methods.

tools/lldb-mi/MICmnMIOutOfBandRecord.cpp
16 ↗(On Diff #31315)

CMICmnMIOutOfBandRecord::MapOutOfBandToText

56 ↗(On Diff #31315)

CMICmnMIOutOfBandRecord::MapOutOfBandToToken

102 ↗(On Diff #31315)

And please follow to the style:

static CMIUtilString
BuildAsyncRecord(CMICmnMIOutOfBandRecord::OutOfBand_e veType)
102 ↗(On Diff #31315)

CMICmnMIOutOfBandRecord::BuildAsyncRecord

This revision now requires changes to proceed.Aug 4 2015, 5:17 AM
brucem updated this revision to Diff 31318.Aug 4 2015, 5:37 AM
brucem edited edge metadata.

Update for formatting fix.

brucem marked an inline comment as done.Aug 4 2015, 5:43 AM
brucem added inline comments.
tools/lldb-mi/MICmnMIOutOfBandRecord.cpp
16 ↗(On Diff #31318)

The style in LLDB itself appears to be to using static file-scope functions when appropriate.

I did the same thing in my previous patch which simplified CMICmnMIResultRecord.

Making this a static function might lead to people prefixing the invocation with the class name, which would be needlessly verbose. Making it a private member function might also lead to, as it has done to date, to people using member variables for strange reasons rather than passing parameters.

Hopefully, there's no need for the internal implementation details of this class to bleed out into the header file.

56 ↗(On Diff #31318)

As above.

102 ↗(On Diff #31318)

As above.

ki.stfu accepted this revision.Aug 4 2015, 6:02 AM
ki.stfu edited edge metadata.
This revision is now accepted and ready to land.Aug 4 2015, 6:02 AM
This revision was automatically updated to reflect the committed changes.