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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
