[Lldb-commits] [PATCH] D100965: [lldb] Refactor argument group by SourceLocationSpec (NFCI)

Wed Apr 28 03:19:42 PDT 2021

teemperor added a comment.

In D100965#2720235 <https://reviews.llvm.org/D100965#2720235>, @jingham wrote:

> In D100965#2716239 <https://reviews.llvm.org/D100965#2716239>, @teemperor wrote:
>
>> I am wondering how SourceLocationSpec is related to lldb's Declaration class (which is FileSpec + line + column and describes a location in source code)? It seems like the current SourceLocationSpec is just a Declaration with the two additional search variables (and the first iteration was exactly the same as Declaration).
>>
>> Could we maybe turn SourceLocationSpec to store a lldb::Declaration instead of file/line/column? I'm aware that the Declaration class first needs some cleanup (and a removal of that #ifdef around column....), but right now we are already using Declarations in a bunch of places to describe source locations.
>
> Seems to me Declarations and SourceLocationSpec's are different things.  A Declaration describes just where something is in a source file.  And moreover, there need to be a lot of them, since every function, type etc has one.  So you might be concerned about size for this entity.
>
> A SourceLocationSpec (maybe we should make a better name?) is about specifying how you search for a source location (e.g. to set a breakpoint on it.)  And there are never going to be lots of them in flight, since they are tied to user actions.  So we are fairly free to add extra fields to this if we have other ways of specifying the match to a source location.  OTOH, Declarations don't need "include_inlines" or "exact_match" or anything else we might end up adding to specify how to look for matches against a source line specification somebody used in break set or other places.
>
> So I don't think it makes sense to conflate the two.

My point was that `Declaration` should be a member of `SourceLocationSpec` (which will then be a Declaration + the search parameters). I agree that those two should stay two different classes. From what understand from Ismail this patch is (was?) going in the direction you're describing of having `SourceLocationSpec` taking over `Declaration` (which would then raise the memory concerns you mentioned).

In other words: If the file/line/column members could just be a `Declaration` (assuming we remove that ifdef around Declarations column member) then IMHO this would be nicer. I would also like the idea of maybe typedef'ing the line/column member types in addition to that. Not sure if we need uint32_t for columns or maybe we need one day uint64_t for lines.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100965