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

@@ -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;

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);
 }

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<int, int> 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:

liblangutil/SourceLocation.h

@@ -56,24 +56,32 @@ 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 +117,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;
 }

libsolidity/analysis/ControlFlowAnalyzer.cpp

@@ -163,7 +163,7 @@ void ControlFlowAnalyzer::checkUnreachable(CFGNode const* _entry, CFGNode const*
 	std::set<SourceLocation> unreachable;
 	util::BreadthFirstSearch<CFGNode const*>{{_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);

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;

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;
 
 }

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();

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<CharStream>("", "root.asm");
+	auto root_asm = make_shared<CharStream>("lorem ipsum", "root.asm");
 	_assembly.setSourceLocation({1, 3, root_asm});
 
 	Assembly _subAsm;
-	auto sub_asm = make_shared<CharStream>("", "sub.asm");
+	auto sub_asm = make_shared<CharStream>("lorem ipsum", "sub.asm");
 	_subAsm.setSourceLocation({6, 8, sub_asm});
 	_subAsm.append(Instruction::INVALID);
 	shared_ptr<Assembly> _subAsmPtr = make_shared<Assembly>(_subAsm);

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;
 	}

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<CharStream>("", "source");
+	auto const source = std::make_shared<CharStream>("lorem ipsum", "source");
-	auto const sourceA = std::make_shared<CharStream>("", "sourceA");
+	auto const sourceA = std::make_shared<CharStream>("lorem ipsum", "sourceA");
-	auto const sourceB = std::make_shared<CharStream>("", "sourceB");
+	auto const sourceB = std::make_shared<CharStream>("lorem ipsum", "sourceB");
 
 	BOOST_CHECK(SourceLocation{} == SourceLocation{});
 	BOOST_CHECK((SourceLocation{0, 3, sourceA} != SourceLocation{0, 3, sourceB}));