jingham added inline comments.
================ Comment at: lldb/include/lldb/Symbol/Declaration.h:113 + /// Compares the specification from \a rhs. If the file specifications are + /// equal, then continue to compare the line. + /// ---------------- clayborg wrote: > How about just returning a bool: > ``` > bool FileAndLineEqual(const Declaration &rhs); > ``` That would be great. The whole "Compare" thing for Declarations doesn't make any sense to me. If you were trying to put Declarations in a container that required an ordering??? But that's not what this is for. ================ Comment at: lldb/include/lldb/Symbol/LineEntry.h:140 + AddressRange GetSameLineContiguousAddressRange( + bool include_inlined_functions = false) const; ---------------- clayborg wrote: > jingham wrote: > > 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. > We tried that but the patch becomes a lot bigger if so. Happy to add all the > other changes back, or we can make a new fucntion? Besides the declaration and definition, I only see 5 uses of GetSameLineContiguousAddressRange in lldb, and you've touched three of them. Am I missing something? If not, might as well change the other two. 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
