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

Jim Ingham via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Thu May 2 14:07:01 PDT 2019

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.

  rG LLVM Github Monorepo



More information about the lldb-commits mailing list