This is an archive of the discontinued LLVM Phabricator instance.

[lldb-mi] Simplify MICmnMIResultRecord implementation.
ClosedPublic

Authored by brucem on Jul 9 2015, 2:48 AM.

Details

Summary
  • Remove extraneous members that were just storing temporary values.
  • Correct some comments to not say that veType was a reference.
  • ResultClass_e parameters don't need to be const as they are scalars.
  • Switch from a map with CMIUtilString values to an indexable array of const char*.
  • Make BuildRecordResult 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.

[lldb-mi] Simplify MICmnMIResultRecord implementation.

Diff Detail

Event Timeline

brucem updated this revision to Diff 29305.Jul 9 2015, 2:48 AM
brucem retitled this revision from to [lldb-mi] Simplify MICmnMIResultRecord implementation. * Remove extraneous members that were just storing temporary values. * Correct some comments to not say that veType was a reference. * ResultClass_e parameters don't need to be const as....
brucem updated this object.
brucem added reviewers: abidh, ki.stfu, domipheus.
brucem added a subscriber: lldb-commits.
brucem added a comment.Jul 9 2015, 2:50 AM

This is an example of what I'd like to do more widely. In this case, the generated code is about half the size (not that it really matters, but just a demonstration of the bloat removed).

brucem retitled this revision from [lldb-mi] Simplify MICmnMIResultRecord implementation. * Remove extraneous members that were just storing temporary values. * Correct some comments to not say that veType was a reference. * ResultClass_e parameters don't need to be const as... to [lldb-mi] Simplify MICmnMIResultRecord implementation..Jul 9 2015, 2:50 AM
brucem updated this object.
ki.stfu requested changes to this revision.Jul 9 2015, 4:52 AM
ki.stfu edited edge metadata.
ki.stfu added inline comments.
tools/lldb-mi/MICmnMIResultRecord.cpp
14–51

Revert please. It will not give us any speed-up, but it does an addition of new ResultRecords harder. + It's much simpler to shift all enums by mistake.

68

(R) means 'readonly' variable. i.e. you will not plan to return anything through that variable.

81

(R) means 'readonly' variable. i.e. you will not plan to return anything through that variable.

86–87

Why did you remove the const qualifier?

tools/lldb-mi/MICmnMIResultRecord.h
62–64

What is wrong with const for scalar types? It does a expression stricter.

This revision now requires changes to proceed.Jul 9 2015, 4:52 AM
brucem updated this revision to Diff 30235.Jul 21 2015, 1:58 AM
brucem edited edge metadata.

This restores the (R) to some comments and handles the mapping of
the result class to a string in a better way that will generate
compiler warnings if a new result class is added.

The const qualifiers haven't been re-added after the discussion on
D11049.

Looks good, but how about returning NULL before exiting from MapResultClassToResultClassText?

tools/lldb-mi/MICmnMIResultRecord.cpp
35

here:

assert(0 && "unknown CMICmnMIResultRecord::ResultClass_e");
return NULL;
brucem updated this revision to Diff 30238.Jul 21 2015, 2:41 AM
brucem edited edge metadata.

Updated per review comment.

brucem updated this revision to Diff 30240.Jul 21 2015, 3:27 AM

Minor update for formatting and to change (void) parameter lists.

ki.stfu accepted this revision.Jul 21 2015, 3:55 AM
ki.stfu edited edge metadata.
This revision is now accepted and ready to land.Jul 21 2015, 3:55 AM
This revision was automatically updated to reflect the committed changes.