Page MenuHomePhabricator

Some FormatEntity.cpp cleanup and unit testing
ClosedPublic

Authored by nealsid on Mar 7 2021, 1:27 PM.

Details

Summary

Just fixing a few things I noticed as I am working on another feature for format strings in the prompt: forward decls, adding constexpr constructors, various checks, and unit tests for FormatEntity::Parse and new Definition constructors, etc.

Diff Detail

Event Timeline

nealsid created this revision.Mar 7 2021, 1:27 PM
nealsid requested review of this revision.Mar 7 2021, 1:27 PM
nealsid updated this revision to Diff 328916.Mar 7 2021, 5:44 PM

Rebased on HEAD

nealsid changed the repository for this revision from rLLDB LLDB to rG LLVM Github Monorepo.Mar 7 2021, 6:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2021, 6:25 PM
nealsid updated this revision to Diff 328917.Mar 7 2021, 6:29 PM

Reuploading patch after I incorrectly set project repo.

nealsid edited the summary of this revision. (Show Details)Mar 7 2021, 6:30 PM
teemperor requested changes to this revision.Mar 8 2021, 5:17 AM
teemperor added a subscriber: teemperor.

Thanks for cleaning this up! Are the if (!sc) ... stuff are missing nullptr checks that affect a normal LLDB session (not sure if we can ever have no SymbolContext) or is this just for the unit test?

Anyway, I have some small inline comments but otherwise this looks pretty good to me.

lldb/include/lldb/Core/FormatEntity.h
113

Can you doxygenify this when you anyway touch this code (/// An array of "num_children" Definition entries, before the member)?

144

I believe you could make this far less verbose with delegating constructors and using C++11 default member initializers.

lldb/source/Core/FormatEntity.cpp
1764

One-line if's don't have {} around the body according to LLVM code style (I know this is wrong in a lot of LLDB code, but newer code should follow that style). Just commenting this once but the same thing is happening above/below this change too.

This revision now requires changes to proceed.Mar 8 2021, 5:17 AM

Thanks - all the comments sound good. I'll upload a new patch.

It seems like the pre-merge check failures are because I incorrectly set the project repo when I first created the revision. I noticed in the build commands here: https://buildkite.com/llvm-project/premerge-checks/builds/28649#b755747b-7673-4dbc-9189-2d6bbcd1fbd3 that the cmake command doesn't include 'lldb' as a project. I was able to repro the error on my Debian machine using those commands, and adding lldb to the cmake command seemed to make them go away (tbh, I'm not familiar enough with the normal output of clang-tidy to be sure)

Is there any way to fix that without creating a new revision or would that be easiest? Thanks, sorry about that.

nealsid updated this revision to Diff 329266.Mar 9 2021, 2:44 AM
nealsid marked 3 inline comments as done.Mar 9 2021, 2:48 AM
nealsid added inline comments.
lldb/include/lldb/Core/FormatEntity.h
144

Makes sense - I used default member initializers but once I did that, delegating constructors seemed to add more code back in :) So I just stuck with the member initializers.

lldb/source/Core/FormatEntity.cpp
1764

Done, here and in other spots I noticed.

nealsid marked 2 inline comments as done.Mar 9 2021, 11:51 AM

Thanks for cleaning this up! Are the if (!sc) ... stuff are missing nullptr checks that affect a normal LLDB session (not sure if we can ever have no SymbolContext) or is this just for the unit test?

Anyway, I have some small inline comments but otherwise this looks pretty good to me.

Missed this earlier - I think you're right, because the formatting code is used for thread/frame lists & information, so there will be a SymbolContext. But one my planned upcoming changes is to use it in the LLDB prompt, where a target might not be available. And it's also more consistent with formatting code for other Entry types.

nealsid planned changes to this revision.Mar 10 2021, 8:12 PM
nealsid added inline comments.
lldb/source/Core/FormatEntity.cpp
1560

Sorry, just caught this. Am uploading a new diff shortly.

nealsid updated this revision to Diff 329834.Mar 10 2021, 8:26 PM

Fix regression introduced by removing return statement in case where we were able to format a function name.

nealsid updated this revision to Diff 330884.Mar 15 2021, 10:49 PM

Rebased on HEAD

Thanks for cleaning this up! Are the if (!sc) ... stuff are missing nullptr checks that affect a normal LLDB session (not sure if we can ever have no SymbolContext) or is this just for the unit test?

Anyway, I have some small inline comments but otherwise this looks pretty good to me.

Thanks again for the review! I should have addressed everything with the new diff I just uploaded, but I'm still not sure about the pre-merge check failures because it looks (to me) like the build bot should have "lldb" in its cmake command, although I will freely admit zero experience with LLVM's build infrastructure :-)

teemperor requested changes to this revision.Mar 24 2021, 2:14 AM

Sorry for the delay!

It seems like the pre-merge check failures are because I incorrectly set the project repo when I first created the revision. I noticed in the build commands here: https://buildkite.com/llvm-project/premerge-checks/builds/28649#b755747b-7673-4dbc-9189-2d6bbcd1fbd3 that the cmake command doesn't include 'lldb' as a project. I was able to repro the error on my Debian machine using those commands, and adding lldb to the cmake command seemed to make them go away (tbh, I'm not familiar enough with the normal output of clang-tidy to be sure)

You can ignore the clang-tidy errors as they look indeed like a setup problem on the bot. They won't prevent this from being merged.

I think this looks almost ready beside some small problems (some of which are maybe a bit subjective).

lldb/include/lldb/Core/FormatEntity.h
111

You can use \see FormatEntity::Entry::Type and this will be linked in doxygen.

119

Truncated comment? Also thanks for documenting this!

lldb/unittests/Core/FormatEntityTest.cpp
24

I think you can use EXPECT_* in all these tests.

155

You could just use llvm::StringRef here (it's shorter and more descriptive than the auto).

This revision now requires changes to proceed.Mar 24 2021, 2:14 AM
nealsid updated this revision to Diff 333507.Mar 26 2021, 1:19 AM

Thanks for the review - addressed your comments here (I had to rely on Doxygen picking up the type reference automatically because I couldn't get the \see syntax to work correctly, though)

nealsid marked 4 inline comments as done.Mar 26 2021, 1:22 AM
teemperor accepted this revision.Mar 29 2021, 1:24 AM

LGTM, thanks for the patch (and especially the unit test)! Some two nits left but feel free to fix those when merging. If you don't have commit access I can also do that, just let me know.

lldb/include/lldb/Core/FormatEntity.h
120

Missing period

lldb/unittests/Core/FormatEntityTest.cpp
155

I actually meant llvm::StringRef as the whole type (StringRef references are not really useful as StringRef is already a lightweight String reference)

This revision is now accepted and ready to land.Mar 29 2021, 1:24 AM
nealsid updated this revision to Diff 337329.Apr 13 2021, 9:50 PM

CR feedback

nealsid updated this revision to Diff 337331.Apr 13 2021, 9:53 PM

Fixed comment.

nealsid updated this revision to Diff 337334.Apr 13 2021, 10:13 PM
nealsid marked an inline comment as done.

Update type in for loop.

nealsid marked an inline comment as done.Apr 13 2021, 10:24 PM
nealsid added inline comments.
lldb/unittests/Core/FormatEntityTest.cpp
155

Done - kept it const since StringRef appears to depend on that for some of its methods. I was looking through StringRef (and the use of optionals for something else) and as far as I understand, which could be entirely wrong, it's meant to provide a single interface over unowned character data whether it comes from a std::string or char*, and thought using string_view would be good, as well - is moving to C++17 on the roadmap? I know it's one of those things that everyone is in favor of and wants to do but nobody has time for :)

nealsid marked an inline comment as done.Apr 13 2021, 10:27 PM

Thanks for the review, and apologies for the delay in updating the diffs - seems like I haven't even had an hour to sit down at my dev machine for the past couple weeks, much less actually do any coding :) I don't have commit access, so if you could do that, that would be much appreciated.

nealsid updated this revision to Diff 339059.Apr 20 2021, 5:21 PM

Small update of patch.

teemperor accepted this revision.Apr 21 2021, 6:10 AM

Thanks for updating and thanks for the patch! I can land it right now.

lldb/unittests/Core/FormatEntityTest.cpp
155

It's on the roadmap but not sure when that will happen. I think that depends on when the interest in having C++17 features is bigger than the pain it will inflict on people stuck on outdated runtime environments that need bootstrapping for C++17.

This revision was landed with ongoing or failed builds.Apr 21 2021, 6:13 AM
This revision was automatically updated to reflect the committed changes.