JDevlieghere added inline comments.
================ Comment at: lldb/include/lldb/Utility/SourceLocationSpec.h:13-15 +#include "lldb/lldb-defines.h" + +#include "llvm/ADT/Optional.h" ---------------- ================ Comment at: lldb/include/lldb/Utility/SourceLocationSpec.h:108-138 + /// Convert to boolean operator. + /// + /// This allows code to check a SourceLocationSpec object to see if it + /// contains anything valid using code such as: + /// + /// \code + /// SourceLocationSpec location_spec(...); ---------------- I guess this is no longer needed now that the factory guarantees the spec is valid? ================ Comment at: lldb/include/lldb/Utility/SourceLocationSpec.h:183 + + bool HasColumn() const { return m_column.hasValue(); } + ---------------- Is there any value to having this? I assume that if you want to know if there is a column, it's to use it after, so you might as well call get column and use the value if it's set. ================ Comment at: lldb/include/lldb/Utility/SourceLocationSpec.h:195-196 + llvm::Optional<uint16_t> m_column; + bool m_check_inlines; + bool m_exact_match; +}; ---------------- Maybe add a Doxygen comment to specify what these two mean exactly. ================ Comment at: lldb/source/Utility/SourceLocationSpec.cpp:59-61 +bool SourceLocationSpec::operator!=(const SourceLocationSpec &rhs) const { + return !(*this == rhs); +} ---------------- Isn't this the default implementation of `operator !=`? I think we can probably omit it? ================ Comment at: lldb/unittests/Utility/SourceLocationSpecTest.cpp:96 + + // mutating FileSpec + const Inlined, ExactMatch, Line + EXPECT_TRUE( ---------------- ================ Comment at: lldb/unittests/Utility/SourceLocationSpecTest.cpp:136-143 + auto Create = + [](bool check_inlines, bool exact_match, FileSpec fs, uint32_t line, + uint16_t column = LLDB_INVALID_COLUMN_NUMBER) -> SourceLocationSpec { + auto location = SourceLocationSpec::Create(fs, line, column, check_inlines, + exact_match); + lldbassert(location && "Invalid SourceLocationSpec."); + return *location; ---------------- This looks identical to the `Create` above. If it is, make it a static function to avoid code duplication. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100962/new/ https://reviews.llvm.org/D100962 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits