[Lldb-commits] [PATCH] D98153: Some FormatEntity.cpp cleanup and unit testing

[Raphael Isemann via Phabricator via lldb-commits]

teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

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).



================
Comment at: lldb/include/lldb/Core/FormatEntity.h:111
+      const char *string = nullptr;
+      /// The type of the format string (see Type above)
+      const Entry::Type type;
----------------
You can use `\see FormatEntity::Entry::Type` and this will be linked in doxygen.


================
Comment at: lldb/include/lldb/Core/FormatEntity.h:119
+      const Definition *children = nullptr;
+      /// Whether
+      const bool keep_separator = false;
----------------
Truncated comment? Also thanks for documenting this!


================
Comment at: lldb/unittests/Core/FormatEntityTest.cpp:23
+
+  ASSERT_EQ(d.name, "foo");
+  ASSERT_EQ(d.string, nullptr);
----------------
I think you can use `EXPECT_*` in all these tests.


================
Comment at: lldb/unittests/Core/FormatEntityTest.cpp:154
+TEST(FormatEntity, LookupAllEntriesInTree) {
+  for (const auto &testString : lookupStrings) {
+    Entry e;
----------------
You could just use `llvm::StringRef` here (it's shorter and more descriptive than the `auto`).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98153/new/

https://reviews.llvm.org/D98153