[Lldb-commits] [PATCH] D61292: Include inlined functions when figuring out a contiguous address range

[Jim Ingham via Phabricator via lldb-commits]

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