[Lldb-commits] [PATCH] D61292: Include inlined functions when figuring out a contiguous address range

[António Afonso via Phabricator via lldb-commits]

aadsm marked 4 inline comments as done and an inline comment as not done.
aadsm added inline comments.


================
Comment at: lldb/unittests/Symbol/TestLineEntry.cpp:60-87
+#define EXPECTED_NO_ERROR(x)                                                   \
+  if (std::error_code ASSERT_NO_ERROR_ec = x) {                                \
+    llvm::SmallString<128> MessageStorage;                                     \
+    llvm::raw_svector_ostream Message(MessageStorage);                         \
+    Message << #x ": did not return errc::success.\n"                          \
+            << "error number: " << ASSERT_NO_ERROR_ec.value() << "\n"          \
+            << "error message: " << ASSERT_NO_ERROR_ec.message() << "\n";      \
----------------
jingham wrote:
> These defines don't seem specific to this test, is there a more general place you could put them?
I could put it in `TestUtilities.h`?


================
Comment at: lldb/unittests/Symbol/TestLineEntry.cpp:96-122
+llvm::Expected<ModuleSP> LineEntryTest::GetModule() {
+  if (m_module_sp)
+    return m_module_sp;
+
+  std::string yaml = GetInputFilePath("inlined-functions.yaml");
+  llvm::SmallString<128> obj;
+
----------------
jingham wrote:
> It looks like this bit of business has been copied around in a bunch of other tests in the unittest framework.  Could we put this in a common place (like make a LLDBUnitTest : testing::Test class that does this?
How about adding a new function to TestUtilities.cpp named `ReadYAMLObject`?


================
Comment at: lldb/unittests/Symbol/TestLineEntry.cpp:193-194
+        int result = sum2(a, b) + sum2(c, d);
+        return result;
+}
+
----------------
jingham wrote:
> Can you make the return here not trivial (like return result + 5)}  The way you have written the line return line doesn't contribute any code so you would step all the way out to the caller, but it would be good to test that we didn't just extend the range to our caller, i.e. that next on "int result ="... stops at the "return result" which it should do except in your example line 13 contributes no code. 
That's a good idea, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61292