Remove Location::byte_ member and byte() method.
Th Location class has a byte_ member representing a token's byte offset
in the input file. However, it is only used to perform less-than
comparisons, which is inconsistent with the Location::operator<()
method which uses (line, column) pairs instead. Also its value cannot
be properly determined when Location() instances are created outside of
the tokenizer (e.g. in the header checker).
This CL simplifies the class by removing the member entirely, and
adding a proper Location::operator<=() instead.
This is mostly cosmetic: measurements do not show any noticeable change
in performance or memory usage.
Change-Id: I220b34fd8bcaabc7750d3792c8dcea4284cd8a57
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/12540
Reviewed-by: Brett Wilson <brettw@chromium.org>
Commit-Queue: David Turner <digit@google.com>
diff --git a/src/gn/c_include_iterator.cc b/src/gn/c_include_iterator.cc
index faf475d..e4cd7d4 100644
--- a/src/gn/c_include_iterator.cc
+++ b/src/gn/c_include_iterator.cc
@@ -139,11 +139,9 @@
if (type != INCLUDE_NONE) {
include->contents = include_contents;
include->location = LocationRange(
- Location(input_file_, cur_line_number, begin_char,
- -1 /* TODO(scottmg): Is this important? */),
+ Location(input_file_, cur_line_number, begin_char),
Location(input_file_, cur_line_number,
- begin_char + static_cast<int>(include_contents.size()),
- -1 /* TODO(scottmg): Is this important? */));
+ begin_char + static_cast<int>(include_contents.size())));
include->system_style_include = (type == INCLUDE_SYSTEM);
lines_since_last_include_ = 0;
diff --git a/src/gn/header_checker.cc b/src/gn/header_checker.cc
index 32f4d90..8d2d313 100644
--- a/src/gn/header_checker.cc
+++ b/src/gn/header_checker.cc
@@ -47,11 +47,10 @@
input_file.name(), &clone_input_file, &tokens, &parse_root);
clone_input_file->SetContents(input_file.contents());
- return LocationRange(
- Location(clone_input_file, range.begin().line_number(),
- range.begin().column_number(), -1 /* TODO(scottmg) */),
- Location(clone_input_file, range.end().line_number(),
- range.end().column_number(), -1 /* TODO(scottmg) */));
+ return LocationRange(Location(clone_input_file, range.begin().line_number(),
+ range.begin().column_number()),
+ Location(clone_input_file, range.end().line_number(),
+ range.end().column_number()));
}
// Given a reverse dependency chain where the target chain[0]'s includes are
diff --git a/src/gn/location.cc b/src/gn/location.cc
index 95bdd13..cba44c8 100644
--- a/src/gn/location.cc
+++ b/src/gn/location.cc
@@ -12,18 +12,12 @@
Location::Location() = default;
-Location::Location(const InputFile* file,
- int line_number,
- int column_number,
- int byte)
- : file_(file),
- line_number_(line_number),
- column_number_(column_number),
- byte_(byte) {}
+Location::Location(const InputFile* file, int line_number, int column_number)
+ : file_(file), line_number_(line_number), column_number_(column_number) {}
bool Location::operator==(const Location& other) const {
- return other.file_ == file_ && other.line_number_ == line_number_ &&
- other.column_number_ == column_number_;
+ return (file_ == other.file_ && line_number_ == other.line_number_ &&
+ column_number_ == other.column_number_);
}
bool Location::operator!=(const Location& other) const {
@@ -36,6 +30,12 @@
std::tie(other.line_number_, other.column_number_);
}
+bool Location::operator<=(const Location& other) const {
+ DCHECK(file_ == other.file_);
+ return std::tie(line_number_, column_number_) <=
+ std::tie(other.line_number_, other.column_number_);
+}
+
std::string Location::Describe(bool include_column_number) const {
if (!file_)
return std::string();
diff --git a/src/gn/location.h b/src/gn/location.h
index 471391b..79af5dd 100644
--- a/src/gn/location.h
+++ b/src/gn/location.h
@@ -13,17 +13,19 @@
class Location {
public:
Location();
- Location(const InputFile* file, int line_number, int column_number, int byte);
+ Location(const InputFile* file, int line_number, int column_number);
const InputFile* file() const { return file_; }
int line_number() const { return line_number_; }
int column_number() const { return column_number_; }
- int byte() const { return byte_; }
bool is_null() const { return *this == Location(); }
bool operator==(const Location& other) const;
bool operator!=(const Location& other) const;
bool operator<(const Location& other) const;
+ bool operator<=(const Location& other) const;
+ bool operator>(const Location& other) const { return !(*this <= other); }
+ bool operator>=(const Location& other) const { return !(*this < other); }
// Returns a string with the file, line, and (optionally) the character
// offset for this location. If this location is null, returns an empty
@@ -34,7 +36,6 @@
const InputFile* file_ = nullptr; // Null when unset.
int line_number_ = -1; // -1 when unset. 1-based.
int column_number_ = -1; // -1 when unset. 1-based.
- int byte_ = 0; // Index into the buffer, 0-based.
};
// Represents a range in a source file. Used for error reporting.
diff --git a/src/gn/parse_tree.cc b/src/gn/parse_tree.cc
index 83452d0..e9ca8fa 100644
--- a/src/gn/parse_tree.cc
+++ b/src/gn/parse_tree.cc
@@ -111,7 +111,7 @@
value.FindKey(kJsonLocation)->FindKey(kJsonLocationBeginLine)->GetInt();
int column =
value.FindKey(kJsonLocation)->FindKey(kJsonLocationBeginColumn)->GetInt();
- return Location(nullptr, line, column, 0);
+ return Location(nullptr, line, column);
}
void GetCommentsFromJSON(ParseNode* node, const base::Value& value) {
@@ -120,7 +120,7 @@
Location loc = GetBeginLocationFromJSON(value);
auto loc_for = [&loc](int line) {
- return Location(nullptr, loc.line_number() + line, loc.column_number(), 0);
+ return Location(nullptr, loc.line_number() + line, loc.column_number());
};
if (value.FindKey(kJsonBeforeComment)) {
@@ -443,8 +443,7 @@
void AccessorNode::SetNewLocation(int line_number) {
Location old = base_.location();
- base_.set_location(
- Location(old.file(), line_number, old.column_number(), old.byte()));
+ base_.set_location(Location(old.file(), line_number, old.column_number()));
}
bool AccessorNode::ComputeAndValidateListIndex(Scope* scope,
@@ -782,14 +781,12 @@
void FunctionCallNode::SetNewLocation(int line_number) {
Location func_old_loc = function_.location();
Location func_new_loc =
- Location(func_old_loc.file(), line_number, func_old_loc.column_number(),
- func_old_loc.byte());
+ Location(func_old_loc.file(), line_number, func_old_loc.column_number());
function_.set_location(func_new_loc);
Location args_old_loc = args_->Begin().location();
Location args_new_loc =
- Location(args_old_loc.file(), line_number, args_old_loc.column_number(),
- args_old_loc.byte());
+ Location(args_old_loc.file(), line_number, args_old_loc.column_number());
const_cast<Token&>(args_->Begin()).set_location(args_new_loc);
const_cast<Token&>(args_->End()->value()).set_location(args_new_loc);
}
@@ -848,8 +845,7 @@
void IdentifierNode::SetNewLocation(int line_number) {
Location old = value_.location();
- value_.set_location(
- Location(old.file(), line_number, old.column_number(), old.byte()));
+ value_.set_location(Location(old.file(), line_number, old.column_number()));
}
// ListNode -------------------------------------------------------------------
@@ -1135,8 +1131,7 @@
void LiteralNode::SetNewLocation(int line_number) {
Location old = value_.location();
- value_.set_location(
- Location(old.file(), line_number, old.column_number(), old.byte()));
+ value_.set_location(Location(old.file(), line_number, old.column_number()));
}
// UnaryOpNode ----------------------------------------------------------------
diff --git a/src/gn/parse_tree_unittest.cc b/src/gn/parse_tree_unittest.cc
index ed28b54..cc49848 100644
--- a/src/gn/parse_tree_unittest.cc
+++ b/src/gn/parse_tree_unittest.cc
@@ -20,8 +20,8 @@
// Make a pretend parse node with proper tracking that we can blame for the
// given value.
InputFile input_file(SourceFile("//foo"));
- Token base_token(Location(&input_file, 1, 1, 1), Token::IDENTIFIER, "a");
- Token member_token(Location(&input_file, 1, 1, 1), Token::IDENTIFIER, "b");
+ Token base_token(Location(&input_file, 1, 1), Token::IDENTIFIER, "a");
+ Token member_token(Location(&input_file, 1, 1), Token::IDENTIFIER, "b");
AccessorNode accessor;
accessor.set_base(base_token);
diff --git a/src/gn/parser.cc b/src/gn/parser.cc
index 07e9ba3..dbc3d62 100644
--- a/src/gn/parser.cc
+++ b/src/gn/parser.cc
@@ -833,7 +833,7 @@
}
const Location start = node->GetRange().begin();
while (cur_comment < static_cast<int>(line_comment_tokens_.size())) {
- if (start.byte() >= line_comment_tokens_[cur_comment].location().byte()) {
+ if (start >= line_comment_tokens_[cur_comment].location()) {
const_cast<ParseNode*>(node)->comments_mutable()->append_before(
line_comment_tokens_[cur_comment]);
++cur_comment;
@@ -871,7 +871,7 @@
continue;
while (cur_comment >= 0) {
- if (end.byte() <= suffix_comment_tokens_[cur_comment].location().byte()) {
+ if (end <= suffix_comment_tokens_[cur_comment].location()) {
const_cast<ParseNode*>(*i)->comments_mutable()->append_suffix(
suffix_comment_tokens_[cur_comment]);
--cur_comment;
diff --git a/src/gn/scope_unittest.cc b/src/gn/scope_unittest.cc
index af39a9d..9f82685 100644
--- a/src/gn/scope_unittest.cc
+++ b/src/gn/scope_unittest.cc
@@ -49,7 +49,7 @@
// Make a pretend parse node with proper tracking that we can blame for the
// given value.
InputFile input_file(SourceFile("//foo"));
- Token assignment_token(Location(&input_file, 1, 1, 1), Token::STRING,
+ Token assignment_token(Location(&input_file, 1, 1), Token::STRING,
"\"hello\"");
LiteralNode assignment;
assignment.set_value(assignment_token);
@@ -237,7 +237,7 @@
// Make a pretend parse node with proper tracking that we can blame for the
// given value.
InputFile input_file(SourceFile("//foo"));
- Token assignment_token(Location(&input_file, 1, 1, 1), Token::STRING,
+ Token assignment_token(Location(&input_file, 1, 1), Token::STRING,
"\"hello\"");
LiteralNode assignment;
assignment.set_value(assignment_token);
@@ -272,7 +272,7 @@
// Make a pretend parse node with proper tracking that we can blame for the
// given value.
InputFile input_file(SourceFile("//foo"));
- Token assignment_token(Location(&input_file, 1, 1, 1), Token::STRING,
+ Token assignment_token(Location(&input_file, 1, 1), Token::STRING,
"\"hello\"");
LiteralNode assignment;
assignment.set_value(assignment_token);
diff --git a/src/gn/string_utils.cc b/src/gn/string_utils.cc
index bff9e8f..bf8f8ba 100644
--- a/src/gn/string_utils.cc
+++ b/src/gn/string_utils.cc
@@ -28,13 +28,10 @@
// The "+1" is skipping over the " at the beginning of the token.
int int_offset = static_cast<int>(offset);
Location begin_loc(token.location().file(), token.location().line_number(),
- token.location().column_number() + int_offset + 1,
- token.location().byte() + int_offset + 1);
- Location end_loc(
- token.location().file(), token.location().line_number(),
- token.location().column_number() + int_offset + 1 +
- static_cast<int>(size),
- token.location().byte() + int_offset + 1 + static_cast<int>(size));
+ token.location().column_number() + int_offset + 1);
+ Location end_loc(token.location().file(), token.location().line_number(),
+ token.location().column_number() + int_offset + 1 +
+ static_cast<int>(size));
return Err(LocationRange(begin_loc, end_loc), msg, help);
}
diff --git a/src/gn/token.h b/src/gn/token.h
index 6a90872..9a2cfca 100644
--- a/src/gn/token.h
+++ b/src/gn/token.h
@@ -70,8 +70,7 @@
return LocationRange(
location_,
Location(location_.file(), location_.line_number(),
- location_.column_number() + static_cast<int>(value_.size()),
- location_.byte() + static_cast<int>(value_.size())));
+ location_.column_number() + static_cast<int>(value_.size())));
}
// Helper functions for comparing this token to something.
diff --git a/src/gn/tokenizer.cc b/src/gn/tokenizer.cc
index a2f0034..b68be3c 100644
--- a/src/gn/tokenizer.cc
+++ b/src/gn/tokenizer.cc
@@ -389,8 +389,7 @@
}
Location Tokenizer::GetCurrentLocation() const {
- return Location(input_file_, line_number_, column_number_,
- static_cast<int>(cur_));
+ return Location(input_file_, line_number_, column_number_);
}
Err Tokenizer::GetErrorForInvalidToken(const Location& location) const {
diff --git a/src/gn/tokenizer_unittest.cc b/src/gn/tokenizer_unittest.cc
index d740d86..e3e0542 100644
--- a/src/gn/tokenizer_unittest.cc
+++ b/src/gn/tokenizer_unittest.cc
@@ -127,10 +127,10 @@
std::vector<Token> results = Tokenizer::Tokenize(&input, &err);
ASSERT_EQ(4u, results.size());
- ASSERT_TRUE(results[0].location() == Location(&input, 1, 1, 1));
- ASSERT_TRUE(results[1].location() == Location(&input, 1, 3, 3));
- ASSERT_TRUE(results[2].location() == Location(&input, 1, 5, 5));
- ASSERT_TRUE(results[3].location() == Location(&input, 2, 3, 8));
+ ASSERT_TRUE(results[0].location() == Location(&input, 1, 1));
+ ASSERT_TRUE(results[1].location() == Location(&input, 1, 3));
+ ASSERT_TRUE(results[2].location() == Location(&input, 1, 5));
+ ASSERT_TRUE(results[3].location() == Location(&input, 2, 3));
}
TEST(Tokenizer, ByteOffsetOfNthLine) {
