From 76cfe4e2ced4a7368853a04a80a338c141fcde60 Mon Sep 17 00:00:00 2001 From: alex Date: Mon, 3 Feb 2020 08:04:21 +0100 Subject: [PATCH] Replaced SourceLocation::isEmpty() with isValid() and hasText(). The function SourceLocation::isEmpty() had somewhat dual role. Sometimes it indicates that the SourceLocation is invalid. Sometimes it means that there is no corresponding source text. Hence the proposal is to replace it with two functions, isValid() and hasText(). I also removed Scanner::sourceAt(). (Do we have a rule of thumb to remove unused code?) Since hasText() checks that start and end are valid indices for source, I adjusted a couple of tests to avoid empty source strings. --- libevmasm/Assembly.cpp | 10 +++++----- liblangutil/Exceptions.cpp | 4 ++-- liblangutil/Scanner.h | 6 ------ liblangutil/SourceLocation.h | 21 ++++++++++++++------ libsolidity/analysis/ControlFlowAnalyzer.cpp | 2 +- libsolidity/analysis/TypeChecker.cpp | 2 +- libsolidity/ast/AsmJsonImporter.cpp | 2 +- libyul/AsmParser.h | 2 +- test/libevmasm/Assembler.cpp | 4 ++-- test/libevmasm/Optimiser.cpp | 2 +- test/liblangutil/SourceLocation.cpp | 6 +++--- 11 files changed, 32 insertions(+), 29 deletions(-) diff --git a/libevmasm/Assembly.cpp b/libevmasm/Assembly.cpp index adc7f5b14..10503c484 100644 --- a/libevmasm/Assembly.cpp +++ b/libevmasm/Assembly.cpp @@ -43,7 +43,7 @@ AssemblyItem const& Assembly::append(AssemblyItem const& _i) assertThrow(m_deposit >= 0, AssemblyException, "Stack underflow."); m_deposit += _i.deposit(); m_items.emplace_back(_i); - if (m_items.back().location().isEmpty() && !m_currentSourceLocation.isEmpty()) + if (!m_items.back().location().isValid() && m_currentSourceLocation.isValid()) m_items.back().setLocation(m_currentSourceLocation); m_items.back().m_modifierDepth = m_currentModifierDepth; return m_items.back(); @@ -69,7 +69,7 @@ namespace string locationFromSources(StringMap const& _sourceCodes, SourceLocation const& _location) { - if (_location.isEmpty() || !_location.source.get() || _sourceCodes.empty() || _location.start >= _location.end || _location.start < 0) + if (!_location.hasText() || _sourceCodes.empty()) return ""; auto it = _sourceCodes.find(_location.source->name()); @@ -97,7 +97,7 @@ public: void feed(AssemblyItem const& _item) { - if (!_item.location().isEmpty() && _item.location() != m_location) + if (_item.location().isValid() && _item.location() != m_location) { flush(); m_location = _item.location(); @@ -141,12 +141,12 @@ public: void printLocation() { - if (!m_location.source && m_location.isEmpty()) + if (!m_location.isValid()) return; m_out << m_prefix << " /*"; if (m_location.source) m_out << " \"" + m_location.source->name() + "\""; - if (!m_location.isEmpty()) + if (m_location.hasText()) m_out << ":" << to_string(m_location.start) + ":" + to_string(m_location.end); m_out << " " << locationFromSources(m_sourceCodes, m_location); m_out << " */" << endl; diff --git a/liblangutil/Exceptions.cpp b/liblangutil/Exceptions.cpp index 59d81b6e6..f035f5044 100644 --- a/liblangutil/Exceptions.cpp +++ b/liblangutil/Exceptions.cpp @@ -51,7 +51,7 @@ Error::Error(Type _type, SourceLocation const& _location, string const& _descrip break; } - if (!_location.isEmpty()) + if (_location.isValid()) *this << errinfo_sourceLocation(_location); if (!_description.empty()) *this << util::errinfo_comment(_description); @@ -60,7 +60,7 @@ Error::Error(Type _type, SourceLocation const& _location, string const& _descrip Error::Error(Error::Type _type, std::string const& _description, SourceLocation const& _location): Error(_type) { - if (!_location.isEmpty()) + if (_location.isValid()) *this << errinfo_sourceLocation(_location); *this << util::errinfo_comment(_description); } diff --git a/liblangutil/Scanner.h b/liblangutil/Scanner.h index 77af26357..1e08e0b6e 100644 --- a/liblangutil/Scanner.h +++ b/liblangutil/Scanner.h @@ -167,12 +167,6 @@ public: /// Do only use in error cases, they are quite expensive. std::string lineAtPosition(int _position) const { return m_source->lineAtPosition(_position); } std::tuple translatePositionToLineColumn(int _position) const { return m_source->translatePositionToLineColumn(_position); } - std::string sourceAt(SourceLocation const& _location) const - { - solAssert(!_location.isEmpty(), ""); - solAssert(m_source.get() == _location.source.get(), "CharStream memory locations must match."); - return m_source->source().substr(_location.start, _location.end - _location.start); - } ///@} private: diff --git a/liblangutil/SourceLocation.h b/liblangutil/SourceLocation.h index 46808f8e0..e6f97942e 100644 --- a/liblangutil/SourceLocation.h +++ b/liblangutil/SourceLocation.h @@ -56,24 +56,33 @@ struct SourceLocation inline bool contains(SourceLocation const& _other) const { - if (isEmpty() || _other.isEmpty() || source.get() != _other.source.get()) + if (!hasText() || !_other.hasText() || source.get() != _other.source.get()) return false; return start <= _other.start && _other.end <= end; } inline bool intersects(SourceLocation const& _other) const { - if (isEmpty() || _other.isEmpty() || source.get() != _other.source.get()) + if (!hasText() || !_other.hasText() || source.get() != _other.source.get()) return false; return _other.start < end && start < _other.end; } - bool isEmpty() const { return start == -1 && end == -1; } + bool isValid() const { return source || start != -1 || end != -1; } + + bool hasText() const + { + return + source && + 0 <= start && + start <= end && + end <= int(source->source().length()); + } std::string text() const { assertThrow(source, SourceLocationError, "Requested text from null source."); - assertThrow(!isEmpty(), SourceLocationError, "Requested text from empty source location."); + assertThrow(0 <= start, SourceLocationError, "Invalid source location."); assertThrow(start <= end, SourceLocationError, "Invalid source location."); assertThrow(end <= int(source->source().length()), SourceLocationError, "Invalid source location."); return source->source().substr(start, end - start); @@ -109,13 +118,13 @@ SourceLocation const parseSourceLocation(std::string const& _input, std::string /// Stream output for Location (used e.g. in boost exceptions). inline std::ostream& operator<<(std::ostream& _out, SourceLocation const& _location) { - if (_location.isEmpty()) + if (!_location.isValid()) return _out << "NO_LOCATION_SPECIFIED"; if (_location.source) _out << _location.source->name(); - _out << "[" << _location.start << "," << _location.end << ")"; + _out << "[" << _location.start << "," << _location.end << "]"; return _out; } diff --git a/libsolidity/analysis/ControlFlowAnalyzer.cpp b/libsolidity/analysis/ControlFlowAnalyzer.cpp index a774dee61..c8a824c54 100644 --- a/libsolidity/analysis/ControlFlowAnalyzer.cpp +++ b/libsolidity/analysis/ControlFlowAnalyzer.cpp @@ -163,7 +163,7 @@ void ControlFlowAnalyzer::checkUnreachable(CFGNode const* _entry, CFGNode const* std::set unreachable; util::BreadthFirstSearch{{_exit, _revert}}.run( [&](CFGNode const* _node, auto&& _addChild) { - if (!reachable.count(_node) && !_node->location.isEmpty()) + if (!reachable.count(_node) && _node->location.isValid()) unreachable.insert(_node->location); for (CFGNode const* entry: _node->entries) _addChild(entry); diff --git a/libsolidity/analysis/TypeChecker.cpp b/libsolidity/analysis/TypeChecker.cpp index 82a362989..e4aa399d8 100644 --- a/libsolidity/analysis/TypeChecker.cpp +++ b/libsolidity/analysis/TypeChecker.cpp @@ -2762,7 +2762,7 @@ bool TypeChecker::visit(Identifier const& _identifier) SecondarySourceLocation ssl; for (Declaration const* declaration: annotation.overloadedDeclarations) - if (declaration->location().isEmpty()) + if (!declaration->location().isValid()) { // Try to re-construct function definition string description; diff --git a/libsolidity/ast/AsmJsonImporter.cpp b/libsolidity/ast/AsmJsonImporter.cpp index 8f4559a55..a2ff7298b 100644 --- a/libsolidity/ast/AsmJsonImporter.cpp +++ b/libsolidity/ast/AsmJsonImporter.cpp @@ -54,7 +54,7 @@ T AsmJsonImporter::createAsmNode(Json::Value const& _node) { T r; r.location = createSourceLocation(_node); - astAssert(!r.location.isEmpty() || !r.location.source, "Invalid source location in Asm AST"); + astAssert(!r.location.isValid() || r.location.hasText(), "Invalid source location in Asm AST"); return r; } diff --git a/libyul/AsmParser.h b/libyul/AsmParser.h index 8abdcc91a..42663ae0a 100644 --- a/libyul/AsmParser.h +++ b/libyul/AsmParser.h @@ -65,7 +65,7 @@ protected: { T r; r.location = _loc; - if (r.location.isEmpty()) + if (!r.location.hasText()) { r.location.start = position(); r.location.end = endPosition(); diff --git a/test/libevmasm/Assembler.cpp b/test/libevmasm/Assembler.cpp index 31b30dd45..eac83e1bb 100644 --- a/test/libevmasm/Assembler.cpp +++ b/test/libevmasm/Assembler.cpp @@ -51,11 +51,11 @@ BOOST_AUTO_TEST_SUITE(Assembler) BOOST_AUTO_TEST_CASE(all_assembly_items) { Assembly _assembly; - auto root_asm = make_shared("", "root.asm"); + auto root_asm = make_shared("lorem ipsum", "root.asm"); _assembly.setSourceLocation({1, 3, root_asm}); Assembly _subAsm; - auto sub_asm = make_shared("", "sub.asm"); + auto sub_asm = make_shared("lorem ipsum", "sub.asm"); _subAsm.setSourceLocation({6, 8, sub_asm}); _subAsm.append(Instruction::INVALID); shared_ptr _subAsmPtr = make_shared(_subAsm); diff --git a/test/libevmasm/Optimiser.cpp b/test/libevmasm/Optimiser.cpp index 7a3125bfd..4d93f4e76 100644 --- a/test/libevmasm/Optimiser.cpp +++ b/test/libevmasm/Optimiser.cpp @@ -72,7 +72,7 @@ namespace for (AssemblyItem const& item: output) { - BOOST_CHECK(item == Instruction::POP || !item.location().isEmpty()); + BOOST_CHECK(item == Instruction::POP || item.location().isValid()); } return output; } diff --git a/test/liblangutil/SourceLocation.cpp b/test/liblangutil/SourceLocation.cpp index 8fe4740fd..c5b47d0f6 100644 --- a/test/liblangutil/SourceLocation.cpp +++ b/test/liblangutil/SourceLocation.cpp @@ -33,9 +33,9 @@ BOOST_AUTO_TEST_SUITE(SourceLocationTest) BOOST_AUTO_TEST_CASE(test_fail) { - auto const source = std::make_shared("", "source"); - auto const sourceA = std::make_shared("", "sourceA"); - auto const sourceB = std::make_shared("", "sourceB"); + auto const source = std::make_shared("lorem ipsum", "source"); + auto const sourceA = std::make_shared("lorem ipsum", "sourceA"); + auto const sourceB = std::make_shared("lorem ipsum", "sourceB"); BOOST_CHECK(SourceLocation{} == SourceLocation{}); BOOST_CHECK((SourceLocation{0, 3, sourceA} != SourceLocation{0, 3, sourceB}));