This is an archive of the discontinued LLVM Phabricator instance.

[lldb][NFC] Minor code cleanup in Editline.cpp
ClosedPublic

Authored by gedatsu217 on Feb 27 2020, 12:43 PM.

Details

Diff Detail

Event Timeline

gedatsu217 created this revision.Feb 27 2020, 12:43 PM

Thanks for the patch Shu! Next time, please upload the patch with full context. (see https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface).

lldb/source/Host/common/Editline.cpp
307 ↗(On Diff #247070)

Do we need the (int) cast? If the compiler complains when you remove it, I think a better alternative would be to use std::max<int>(3, ...).

gedatsu217 updated this revision to Diff 247508.Mar 1 2020, 9:00 AM

I fixed a little according to your instruction, and I made an additional revision. (I revised type inference.)

JDevlieghere added inline comments.Mar 2 2020, 3:22 PM
source/Host/common/Editline.cpp
150

Please run clang format over your patch. If you add the following file [1] to your path, you can run git clang-format and it will only reformat the staged changes.

[1] https://github.com/llvm/llvm-project/blob/master/clang/tools/clang-format/git-clang-format

Sorry, I revised it.

JDevlieghere accepted this revision.Mar 3 2020, 6:12 PM

Thank you. LGTM

This revision is now accepted and ready to land.Mar 3 2020, 6:12 PM
gedatsu217 added a comment.EditedMar 4 2020, 8:34 AM

What should I do after this? I think I don't have the right to commit.

What should I do after this? I think I don't have the right to commit.

I can commit it for you, but @teemperor asked me to hold off until he's had the chance to take a look.

teemperor retitled this revision from Cleaning up the codes to [lldb][NFC] Minor code cleanup in Editline.cpp.Mar 5 2020, 1:34 PM
teemperor edited the summary of this revision. (Show Details)
teemperor set the repository for this revision to rLLDB LLDB.
Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2020, 1:34 PM

Actually this LGTM (Jonas already caught the thing I wanted to complain about) but the commit message needs some improvements:

  • The title should contain "[NFC]" (which is short for "No functional change" and just means that you didn't change any functionality here. Makes it easier for people looking for bugs).
  • Put a "[lldb]" in front of the title to show that this is just an LLDB commit.
  • Maybe a more descriptive title between those two tags such as "Minor code cleanup in Editline.cpp". It should be clear from the title what this commit is doing.

I'll do that here but in the next commit you are on your own for making a good title :).

teemperor accepted this revision.Mar 5 2020, 1:35 PM
This revision was automatically updated to reflect the committed changes.