jingham requested changes to this revision.
jingham added a comment.
This revision now requires changes to proceed.
This makes sense to me. Pinning the extension to the call site prevented all
the cases I could think of where this might go wrong.
I had a bunch of small comments inline.
================
Comment at: lldb/include/lldb/Symbol/Block.h:191-196
+ /// @param[in] file
+ /// the file to match at the call site.
+ ///
+ /// @param[in] line
+ /// the line to match at the call site.
+ ///
----------------
The docs don't match the function signature.
================
Comment at: lldb/include/lldb/Symbol/Declaration.h:112-113
+ ///
+ /// Compares the specification from \a rhs. If the file specifications are
+ /// equal, then continue to compare the line.
+ ///
----------------
The argument is not called "rhs" anymore.
I'm not at all sure of the value of returning > < here. If you get > or < you
have no way of knowing if that's because the files don't match, in which case
so far as I can see the ordering is not useful, or if they are in the same file
but with greater or lesser line number. To actually use the < or > you would
have to go back and compare the files again anyway, so for all practical
purposes you're really just returning a bool... And certainly that's how you
are using it.
You were probably just copying the behavior from the function above, but that
one doesn't make much sense either...
================
Comment at: lldb/include/lldb/Symbol/LineEntry.h:135
+ /// @param[in] include_inlined_functions
+ /// Whether to include inlined functions at the same line are not.
+ ///
----------------
are -> or
================
Comment at: lldb/include/lldb/Symbol/LineEntry.h:140
+ AddressRange GetSameLineContiguousAddressRange(
+ bool include_inlined_functions = false) const;
----------------
We try to avoid default arguments unless there's a good reason, and in this
case it looks like you pass the argument explicitly in almost all the uses, so
I would make this a regular argument.
================
Comment at: lldb/source/Core/AddressRange.cpp:131
+ lhs_end_addr != rhs_base_addr)
+ // The ranges don't intercept at all on the right side of this range.
+ return false;
----------------
intercept -> intersect
================
Comment at: lldb/source/Symbol/LineEntry.cpp:199
+ auto symbol_context_scope = lldb::eSymbolContextLineEntry;
+ Declaration find_call_site(original_file, line);
+ if (include_inlined_functions)
----------------
start_call_site would be a clearer name for this. find_call_site doesn't say
what it is.
================
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";
\
----------------
These defines don't seem specific to this test, is there a more general place
you could put them?
================
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;
+
----------------
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?
================
Comment at: lldb/unittests/Symbol/TestLineEntry.cpp:193-194
+ int result = sum2(a, b) + sum2(c, d);
+ return result;
+}
+
----------------
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.
================
Comment at: lldb/unittests/Symbol/TestLineEntry.cpp:279
+*/
\ No newline at end of file
----------------
Add newline.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61292/new/
https://reviews.llvm.org/D61292
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
