Page MenuHomePhabricator

[lldb/Core] Add SourceLocationSpec class (NFC)
ClosedPublic

Authored by mib on Apr 21 2021, 8:18 AM.

Details

Summary

A source location specifier class that holds a Declaration object containing
a FileSpec with line and column information. The column line is optional.
It also holds search flags that can be fetched by resolvers to look inlined
declarations and/or exact matches.

It describes a specific location in a source file and allows the user
to perform checks and comparaisons between multiple instances of that class.

Signed-off-by: Med Ismail Bennani <medismail.bennani@gmail.com>

Diff Detail

Event Timeline

mib created this revision.Apr 21 2021, 8:18 AM
mib requested review of this revision.Apr 21 2021, 8:18 AM
JDevlieghere added inline comments.Apr 21 2021, 8:41 AM
lldb/include/lldb/Utility/SourceLocationSpec.h
12–19 ↗(On Diff #339246)
90 ↗(On Diff #339246)
126 ↗(On Diff #339246)

The class looks mostly immutable (which is good) so I think we can omit this.

153 ↗(On Diff #339246)
160 ↗(On Diff #339246)

Would it make sense to merge this into Equal and replace the bool with an enum value that represents

  • file + line must match,
  • file + line + col must match,
  • file + line + col must match if lhs has it?
178 ↗(On Diff #339246)

If 0 is not a valid column, we should enforce that rather than having two ways of representing it. Can we add an assert to the ctor and make it a precondition for the class?

184 ↗(On Diff #339246)

Are there situations where the line is optional too?

lldb/unittests/Utility/SourceLocationSpecTest.cpp
1–2 ↗(On Diff #339246)
mib marked an inline comment as done.Apr 21 2021, 8:45 AM
mib added inline comments.
lldb/include/lldb/Utility/SourceLocationSpec.h
184 ↗(On Diff #339246)

Technically, m_line could be equal to 0 (if there is no line information) but that would basically mean this class is a FileSpec container.

I guess I can add an assert in the constructor checking if it's not null.

JDevlieghere added inline comments.Apr 21 2021, 8:53 AM
lldb/include/lldb/Utility/SourceLocationSpec.h
184 ↗(On Diff #339246)

Indeed. Another option would be to make it a precondition that a SourceLocationSpec is always valid, assuming that unlike the column number, the file and line are usually valid. One way to do that is by adding asserts to the ctor or if this class is often created from user input, then I'd make the ctor private and add a static CreateSoureLocationSpec factory, that checks our preconditions and returns an Expected/Optional. That has the advantage of not having the change all the call sites for the few scenarios where those are actually invalid. This has the advantage of paying the price upfront when creating the LocationSpec rather than having to check in every place it's used.

mib marked 9 inline comments as done.Apr 22 2021, 11:44 AM
mib added inline comments.
lldb/include/lldb/Utility/SourceLocationSpec.h
160 ↗(On Diff #339246)

I simplified the API removing the Match method since it's the same as calling Exact with bool full = false.

mib updated this revision to Diff 339743.Apr 22 2021, 11:45 AM
mib marked an inline comment as done.
mib edited the summary of this revision. (Show Details)

Addressed @JDevlieghere comments.

mib updated this revision to Diff 339867.Apr 22 2021, 9:30 PM
mib edited the summary of this revision. (Show Details)
  • Move boolean attributes to the end of the class.
  • Fix deleted default constructor issue.
JDevlieghere added inline comments.Apr 23 2021, 8:32 AM
lldb/include/lldb/Utility/SourceLocationSpec.h
13–15 ↗(On Diff #339867)
108–138 ↗(On Diff #339867)

I guess this is no longer needed now that the factory guarantees the spec is valid?

183 ↗(On Diff #339867)

Is there any value to having this? I assume that if you want to know if there is a column, it's to use it after, so you might as well call get column and use the value if it's set.

195–196 ↗(On Diff #339867)

Maybe add a Doxygen comment to specify what these two mean exactly.

lldb/source/Utility/SourceLocationSpec.cpp
59–61 ↗(On Diff #339867)

Isn't this the default implementation of operator !=? I think we can probably omit it?

lldb/unittests/Utility/SourceLocationSpecTest.cpp
96 ↗(On Diff #339867)
136–143 ↗(On Diff #339867)

This looks identical to the Create above. If it is, make it a static function to avoid code duplication.

mib marked 7 inline comments as done.Apr 23 2021, 9:28 AM
mib added inline comments.
lldb/source/Utility/SourceLocationSpec.cpp
59–61 ↗(On Diff #339867)

No, it has to be implemented separately.

mib marked an inline comment as done.Apr 23 2021, 10:56 AM
mib updated this revision to Diff 340117.Apr 23 2021, 11:18 AM

Address @JDevlieghere comments.

This revision is now accepted and ready to land.Apr 23 2021, 12:32 PM
mib updated this revision to Diff 340202.Apr 23 2021, 4:35 PM

Fix unit test failure.

mib updated this revision to Diff 340210.Apr 23 2021, 5:27 PM
mib updated this revision to Diff 341734.Apr 29 2021, 7:11 PM
mib edited the summary of this revision. (Show Details)

Group the FileSpec, the Line and Column number into a Declaration.
Remove factory function to use constructor and added an IsValid method (and boolean operator)

mib updated this revision to Diff 341736.Apr 29 2021, 7:12 PM
mib updated this revision to Diff 341738.
mib requested review of this revision.Apr 29 2021, 7:14 PM
mib retitled this revision from [lldb/Utility] Add SourceLocationSpec class (NFC) to [lldb/Core] Add SourceLocationSpec class (NFC).
mib updated this revision to Diff 341752.Apr 29 2021, 8:07 PM

Wrap SourceLocationSpec column argument into an llvm::Optional

JDevlieghere accepted this revision.Apr 29 2021, 8:09 PM

LGTM

lldb/unittests/Utility/CMakeLists.txt
41 ↗(On Diff #341738)

Let's exclude the change to this file from this patch.

This revision is now accepted and ready to land.Apr 29 2021, 8:09 PM
JDevlieghere added inline comments.Apr 29 2021, 8:17 PM
lldb/source/Core/SourceLocationSpec.cpp
49–62

As this is only using members form Declaration, should it be implemented there?

73–78

Looks like Declaration has a Dump method, so I guess this should just call that.

mib updated this revision to Diff 341763.Apr 29 2021, 9:06 PM
mib marked 3 inline comments as done.

Address @JDevlieghere comments.

Harbormaster completed remote builds in B101807: Diff 341738.
mib updated this revision to Diff 341774.Apr 29 2021, 10:50 PM

A few more comments about things I missed in the previous review.

lldb/source/Core/SourceLocationSpec.cpp
58

Should this take a StreamString so we can write to the stream directory? Or alternatively change the dump method in declaration to also take a raw_ostream? Both would also improve the operator<<.

66–72

This should probably return a std::string instead. Now we're going to keep the string around forever for every invocation of this method.

mib updated this revision to Diff 342068.Apr 30 2021, 3:32 PM
mib marked 2 inline comments as done.
mib updated this revision to Diff 342073.Apr 30 2021, 3:46 PM
This revision was landed with ongoing or failed builds.May 4 2021, 9:35 AM
This revision was automatically updated to reflect the committed changes.