Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial lexing support for integer literals following #143. #269

Merged
merged 6 commits into from
Feb 19, 2021
Merged

Initial lexing support for integer literals following #143. #269

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
87506d4
Initial lexing support for integer literals following #143.
zygoloid Feb 12, 2021
fcd30d6
Address review feedback.
zygoloid Feb 16, 2021
94b3e72
Switch to using a bitset to determine digit validity.
zygoloid Feb 16, 2021
57a7406
Address review comments.
zygoloid Feb 19, 2021
9ff89cc
Remove redundant assert.
zygoloid Feb 19, 2021
396f487
Switch over two comments so the more detailed comment is closer to the
zygoloid Feb 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
224 changes: 217 additions & 7 deletions lexer/tokenized_buffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@

#include <algorithm>
#include <cmath>
#include <iterator>
#include <string>

#include "llvm/ADT/StringExtras.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/StringSwitch.h"
#include "llvm/ADT/Twine.h"
#include "llvm/Support/ErrorHandling.h"
#include "llvm/Support/Format.h"
#include "llvm/Support/FormatVariadic.h"
Expand All @@ -20,7 +22,17 @@ namespace Carbon {

static auto TakeLeadingIntegerLiteral(llvm::StringRef source_text)
-> llvm::StringRef {
return source_text.take_while([](char c) { return llvm::isDigit(c); });
if (source_text.empty() || !llvm::isDigit(source_text.front()))
return llvm::StringRef();

// Greedily consume all following characters that might be part of an integer
// literal. This allows us to produce better diagnostics on invalid literals.
//
// TODO(zygoloid): Update lexical rules to specify that an integer literal
// cannot be immediately followed by another integer literal or a word.
return source_text.take_while([](char c) {
return llvm::isAlnum(c) || c == '_';
});
}

struct UnmatchedClosing {
Expand All @@ -45,6 +57,79 @@ struct MismatchedClosing {
}
};

struct EmptyDigitSequence {
static constexpr llvm::StringLiteral ShortName =
"syntax-invalid-number";
static constexpr llvm::StringLiteral Message =
"Empty digit sequence in numeric literal.";

struct Substitutions {
};
static auto Format(const Substitutions&) -> std::string {
return Message.str();
}
};

struct InvalidDigit {
static constexpr llvm::StringLiteral ShortName =
"syntax-invalid-number";

struct Substitutions {
char digit;
unsigned radix;
};
static auto Format(const Substitutions &subst) -> std::string {
char digit_str[] = {subst.digit, '\0'};
chandlerc marked this conversation as resolved.
Show resolved Hide resolved
return (llvm::Twine("Invalid digit '") + digit_str + "' in " +
(subst.radix == 2 ? "binary"
: subst.radix == 16 ? "hexadecimal" : "decimal") +
" numeric literal.")
.str();
chandlerc marked this conversation as resolved.
Show resolved Hide resolved
}
};

struct InvalidDigitSeparator {
static constexpr llvm::StringLiteral ShortName =
"syntax-invalid-number";
static constexpr llvm::StringLiteral Message =
"Misplaced digit separator in numeric literal.";

struct Substitutions {
};
static auto Format(const Substitutions&) -> std::string {
return Message.str();
}
};

struct IrregularDigitSeparators {
static constexpr llvm::StringLiteral ShortName =
"syntax-irregular-digit-separators";

struct Substitutions {
unsigned radix;
};
static auto Format(const Substitutions &subst) -> std::string {
assert((subst.radix == 10 || subst.radix == 16) && "unexpected radix");
return (llvm::Twine("Digit separators in ") +
(subst.radix == 10 ? "decimal" : "hexadecimal") +
" should appear every " + (subst.radix == 10 ? "3" : "4") +
" characters from the right.")
.str();
}
};

struct UnknownBaseSpecifier {
static constexpr llvm::StringLiteral ShortName =
"syntax-invalid-number";
static constexpr llvm::StringLiteral Message =
"Unknown base specifier in numeric literal.";

struct Substitutions {};
static auto Format(const Substitutions&) -> std::string {
return Message.str();
}
};

struct UnrecognizedCharacters {
static constexpr llvm::StringLiteral ShortName =
"syntax-unrecognized-characters";
Expand Down Expand Up @@ -153,15 +238,91 @@ class TokenizedBuffer::Lexer {
return false;
}

struct CheckDigitSequenceResult {
bool ok;
bool has_digit_separators = false;
};

auto CheckDigitSequence(llvm::StringRef text, unsigned radix)
-> CheckDigitSequenceResult {
assert((radix == 2 || radix == 10 || radix == 16) && "unknown radix");

if (text.empty()) {
emitter.EmitError<EmptyDigitSequence>(
[&](EmptyDigitSequence::Substitutions &) {});
return {.ok = false};
}

unsigned digit_separators = 0;
chandlerc marked this conversation as resolved.
Show resolved Hide resolved
char max_decimal = (radix == 2) ? '1' : '9';

for (auto it = text.begin(), end = text.end(); it != end; ++it) {
chandlerc marked this conversation as resolved.
Show resolved Hide resolved
char c = *it;
if ((c >= '0' && c <= max_decimal) ||
(radix == 16 && c >= 'A' && c <= 'Z')) {
continue;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it'd be easier to read to use a std::bitset<256> here? Setting aside any performance concerns, above it'd be a bit more code but somewhat obvious code setting up the set. And here it'd just be if (valid_digits.test(static_cast<unsigned_char>(c)) { which at least for me is easier to understand than this logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I've checked and it looks like we generate good enough code for this: https://godbolt.org/z/o79rsz

I mean, I would have liked https://godbolt.org/z/W76Gq6 more, but I don't suppose I can nerd-snipe anyone into getting the optimizer to produce that... :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OMG, why-oh-why did you have to show me how bad these are? I am so sad now.

Anyways, nothing to do here. I think the more data-oriented implementation is a bit easier to read anyways, and we can replace the abstraction if/when desired or meaningful.


if (c == '_') {
// A digit separator cannot appear at the start of a digit sequence,
// next to another digit separator, or at the end.
if (it == text.begin() || it[-1] == '_' || it + 1 == text.end()) {
chandlerc marked this conversation as resolved.
Show resolved Hide resolved
emitter.EmitError<InvalidDigitSeparator>(
[&](InvalidDigitSeparator::Substitutions &) {});
buffer.has_errors = true;
}
++digit_separators;
continue;
}

emitter.EmitError<InvalidDigit>(
[&](InvalidDigit::Substitutions &subst) {
subst.digit = c;
subst.radix = radix;
});
return {.ok = false};
}

if (!digit_separators)
return {.ok = true};

// For decimal and hexadecimal digit sequences, digit separators must form
// groups of 3 or 4 digits (4 or 5 characters), respectively.
if (radix != 2) {
// Check for digit separators in the expected positions.
unsigned stride = (radix == 10 ? 4 : 5);
for (auto pos = text.end(); pos - text.begin() >= stride; /*in loop*/) {
pos -= stride;
if (*pos != '_') {
emitter.EmitError<IrregularDigitSeparators>(
[&](IrregularDigitSeparators::Substitutions &subst) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idle speculation: [&](auto& subt) might be a nice idiom for this since the type is already stated earlier and the replication is not worth a ton. Alternately, I wonder if there is a way to infer the template from the lambda's parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to restructure how EmitError works in general, though as a separate patch -- I think we should be returning the substitutions by value rather than mutating an uninitialized object. But that would remove our ability to use auto. I think it'd also make sense to have an overload that just takes the substitutions directly, for the case where there is no overhead in computing them. (Which is always, when emitting an error, because -- I hope! -- errors are always emitted.)

OK if I defer doing things here to a follow-on patch?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

absolutely

subst.radix = radix;
});
buffer.has_errors = true;
digit_separators = 0;
break;
}
--digit_separators;
}

// Check there weren't any other digit separators.
if (digit_separators) {
emitter.EmitError<IrregularDigitSeparators>(
[&](IrregularDigitSeparators::Substitutions &subst) {
subst.radix = radix;
});
buffer.has_errors = true;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extract this to a helper function? Should also make the conditions a bit simpler:

if (radix != 2 & digit_separators)
  CheckDigitSeparatorSequences(...)`

return {.ok = true, .has_digit_separators = digit_separators}`

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I used a lambda rather than a separate function because this code has invariants that the caller sets up (specifically that it's given the number of digit separators found in the string).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I don't think this helps the readability as much as extracting the function would. It somewhat still forces the reader to work through the long function body.

I'm just suggesting a file-local helper function so I don't think the invariants are too complex? The code even seems to already check them with asserts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, done. The code didn't already check its invariants with asserts, except in one corner case; now that it's (in principle) callable from elsewhere in the file, I've made it do so.


return {.ok = true, .has_digit_separators = true};
}

auto LexIntegerLiteral(llvm::StringRef& source_text) -> bool {
llvm::StringRef int_text = TakeLeadingIntegerLiteral(source_text);
if (int_text.empty()) {
return false;
}
llvm::APInt int_value;
if (int_text.getAsInteger(/*Radix=*/0, int_value)) {
return false;
}

int int_column = current_column;
current_column += int_text.size();
Expand All @@ -171,6 +332,55 @@ class TokenizedBuffer::Lexer {
current_line_info->indent = int_column;
set_indent = true;
}

auto add_error_token = [&] {
buffer.AddToken({
.kind = TokenKind::Error(),
.token_line = current_line,
.column = int_column,
.error_length = static_cast<int32_t>(int_text.size()),
});
buffer.has_errors = true;
};
chandlerc marked this conversation as resolved.
Show resolved Hide resolved

unsigned radix = 10;
llvm::StringRef digits = int_text;
if (int_text.size() >= 2 && int_text[0] == '0') {
if (int_text[1] == 'x') {
radix = 16;
digits = digits.drop_front(2);
} else if (int_text[1] == 'b') {
radix = 2;
digits = digits.drop_front(2);
} else {
emitter.EmitError<UnknownBaseSpecifier>(
[&](UnknownBaseSpecifier::Substitutions &subst) {});
add_error_token();
return true;
chandlerc marked this conversation as resolved.
Show resolved Hide resolved
}
}

llvm::APInt int_value;

if (auto result = CheckDigitSequence(digits, radix); !result.ok) {
add_error_token();
return true;
} else if (result.has_digit_separators) {
chandlerc marked this conversation as resolved.
Show resolved Hide resolved
// TODO(zygoloid): Avoid the memory allocation here.
std::string cleaned;
cleaned.reserve(digits.size());
std::remove_copy_if(digits.begin(), digits.end(),
std::back_inserter(cleaned),
[](char c) { return c == '_'; });
if (llvm::StringRef(cleaned).getAsInteger(radix, int_value)) {
llvm_unreachable("should never fail");
}
} else {
if (digits.getAsInteger(radix, int_value)) {
llvm_unreachable("should never fail");
}
}

auto token = buffer.AddToken({.kind = TokenKind::IntegerLiteral(),
.token_line = current_line,
.column = int_column});
Expand Down Expand Up @@ -417,8 +627,8 @@ auto TokenizedBuffer::GetTokenText(Token token) const -> llvm::StringRef {
return source->Text().slice(token_start, token_stop);
}

// Refer back to the source text to preserve oddities like radix or leading
// 0's the author had.
// Refer back to the source text to preserve oddities like radix or digit
// separators the author included.
if (token_info.kind == TokenKind::IntegerLiteral()) {
auto& line_info = GetLineInfo(token_info.token_line);
int64_t token_start = line_info.start + token_info.column;
Expand Down
92 changes: 91 additions & 1 deletion lexer/tokenized_buffer_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ TEST_F(LexerTest, TracksLinesAndColumns) {
}

TEST_F(LexerTest, HandlesIntegerLiteral) {
auto buffer = Lex("12-578\n 1 2");
auto buffer = Lex("12-578\n 1 2\n0x12_3ABC\n0b10_10_11\n1_234_567");
EXPECT_FALSE(buffer.HasErrors());
ASSERT_THAT(buffer, HasTokens(llvm::ArrayRef<ExpectedToken>{
{.kind = TokenKind::IntegerLiteral(),
Expand All @@ -104,6 +104,21 @@ TEST_F(LexerTest, HandlesIntegerLiteral) {
.column = 6,
.indent_column = 3,
.text = "2"},
{.kind = TokenKind::IntegerLiteral(),
.line = 3,
.column = 1,
.indent_column = 1,
.text = "0x12_3ABC"},
{.kind = TokenKind::IntegerLiteral(),
.line = 4,
.column = 1,
.indent_column = 1,
.text = "0b10_10_11"},
{.kind = TokenKind::IntegerLiteral(),
.line = 5,
.column = 1,
.indent_column = 1,
.text = "1_234_567"},
}));
auto token_12 = buffer.Tokens().begin();
EXPECT_EQ(buffer.GetIntegerLiteral(*token_12), 12);
Expand All @@ -113,6 +128,81 @@ TEST_F(LexerTest, HandlesIntegerLiteral) {
EXPECT_EQ(buffer.GetIntegerLiteral(*token_1), 1);
auto token_2 = buffer.Tokens().begin() + 4;
EXPECT_EQ(buffer.GetIntegerLiteral(*token_2), 2);
auto token_0x12_3abc = buffer.Tokens().begin() + 5;
EXPECT_EQ(buffer.GetIntegerLiteral(*token_0x12_3abc), 0x12'3abc);
auto token_0b10_10_11 = buffer.Tokens().begin() + 6;
EXPECT_EQ(buffer.GetIntegerLiteral(*token_0b10_10_11), 0b10'10'11);
auto token_1_234_567 = buffer.Tokens().begin() + 7;
EXPECT_EQ(buffer.GetIntegerLiteral(*token_1_234_567), 1'234'567);
}

TEST_F(LexerTest, ValidatesBaseSpecifier) {
llvm::StringLiteral valid[] = {
"0", "1", "123456789000000000000000000000000000000000000", //
"0x0123456789ABCDEF", "0x0000000000000000000000000000000", //
"0b10110100101001010", "0b0000000"
};
chandlerc marked this conversation as resolved.
Show resolved Hide resolved
for (llvm::StringLiteral literal : valid) {
auto buffer = Lex(literal);
EXPECT_FALSE(buffer.HasErrors()) << literal;
ASSERT_THAT(buffer, HasTokens(llvm::ArrayRef<ExpectedToken>{
{.kind = TokenKind::IntegerLiteral(),
.line = 1,
.column = 1,
.indent_column = 1,
.text = literal}}));
}

llvm::StringLiteral invalid[] = {
"00", "0X123", "0o123", "0B1", "007", "123L", "123456789A", "0x", "0b",
"0x123abc", "0b011101201001", "0b10A"
};
for (llvm::StringLiteral literal : invalid) {
auto buffer = Lex(literal);
EXPECT_TRUE(buffer.HasErrors()) << literal;
ASSERT_THAT(buffer, HasTokens(llvm::ArrayRef<ExpectedToken>{
{.kind = TokenKind::Error(),
.line = 1,
.column = 1,
.indent_column = 1,
.text = literal}}));
}
}

TEST_F(LexerTest, ValidatesIntegerDigitSeparators) {
llvm::StringLiteral valid[] = {
"1_234", "123_456", "1_234_567", //
"0x1_0000", "0x1000_0000", "0x1_0000_0000", //
"0b1_0_1_0_1_0", "0b111_0000",
};
for (llvm::StringLiteral literal : valid) {
auto buffer = Lex(literal);
EXPECT_FALSE(buffer.HasErrors()) << literal;
ASSERT_THAT(buffer, HasTokens(llvm::ArrayRef<ExpectedToken>{
{.kind = TokenKind::IntegerLiteral(),
.line = 1,
.column = 1,
.indent_column = 1,
.text = literal}}));
}

llvm::StringLiteral invalid[] = {
"12_34", "123_4_6_789", "12_3456_789", "12__345", "1_", //
"0x_1234", "0x123_", "0x12_3", "0x_234_5678", "0x1234_567", //
"0b_10101", "0b1__01", "0b1011_", "0b1_01_01_",
};
for (llvm::StringLiteral literal : invalid) {
auto buffer = Lex(literal);
EXPECT_TRUE(buffer.HasErrors()) << literal;
// We expect to produce a token even for a literal containing invalid digit
// separators, for better error recovery.
ASSERT_THAT(buffer, HasTokens(llvm::ArrayRef<ExpectedToken>{
{.kind = TokenKind::IntegerLiteral(),
.line = 1,
.column = 1,
.indent_column = 1,
.text = literal}}));
}
}

TEST_F(LexerTest, HandlesGarbageCharacters) {
Expand Down