-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[LLDB] Add Lexer (with tests) for DIL (Data Inspection Language). #123521
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.
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
Changes from 4 commits
468f73f
61a2607
5e2ee55
ccf5203
29e9f26
0b33ab7
4103144
29ad86c
41416de
f56c019
5dbf7d2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,131 @@ | ||||||||||||
| //===-- DILLexer.h ----------------------------------------------*- C++ -*-===// | ||||||||||||
| // | ||||||||||||
| // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. | ||||||||||||
| // See https://llvm.org/LICENSE.txt for license information. | ||||||||||||
| // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||||||||||||
| // | ||||||||||||
| //===----------------------------------------------------------------------===// | ||||||||||||
|
|
||||||||||||
| #ifndef LLDB_VALUEOBJECT_DILLEXER_H_ | ||||||||||||
| #define LLDB_VALUEOBJECT_DILLEXER_H_ | ||||||||||||
|
||||||||||||
| #ifndef LLDB_VALUEOBJECT_DILLEXER_H_ | |
| #define LLDB_VALUEOBJECT_DILLEXER_H_ | |
| #ifndef LLDB_VALUEOBJECT_DILLEXER_H | |
| #define LLDB_VALUEOBJECT_DILLEXER_H |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It fits in nicer with the other headers (though maybe you don't need it if you remove the UINT_MAX thing below)
| #include <limits.h> | |
| #include <climits> |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the only remaining use of the "unknown" token is to construct "empty" tokens in the tests. I think all of those cases could be avoided by just declaring the Token variable later in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That works for this PR, but the parser has a token member variable, where it stores the current token it's working on. When I create the parser I have to initialize the member variable token to some value; the only one that makes sense is "unknown".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe that variable should be optional<Token> then? Though I'm not sure why it's needed as things could just call lexer.GetCurrentToken() instead. In either case, I'd like to remove this from this patch as its not needed here. If it turns out to be the best solution to the parsers needs, we can add the extra type then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just thought of an alternative implementation I think I can use that will make this work. :-) I'll test that and if it works I'll get rid of the 'unknown' token type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The unknown token is still here. Are you sure you uploaded the right version of the patch?
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| : m_kind(kind), m_spelling(spelling), m_start_pos(start) {} | |
| : m_kind(kind), m_spelling(std::move(spelling)), m_start_pos(start) {} |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we just have the lexer start at token zero straight away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really...this index indicates which token the parser is currently working on. We shouldn't set the index to zero until the parser asks for the first token (& starts parsing with it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And why is that the case? I don't find this argument particularly convincing. All it does is create a weird "before the first token" state, which you have to account for both in the implementation of the lexer functions and in the functions that use that. I think things would be much simpler if you could just always assume that the lexer has a valid "current" position, and I think that's basically we have the eof pseudo-token. If there was some value in having a "before-the first token" state, then i think it'd be better to have a bof pseudo-token to match that (but I doubt that's the case).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok; the alternative I mentioned above should make this work too (fingers crossed).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I'd hope that all of the UINT_MAX business can go away if we really start at token zero.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A valid token stream should always end in and EOF token, right? If, so you can just return the last element.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| : m_expr(dil_expr), m_lexed_tokens(lexed_tokens), m_tokens_idx(UINT_MAX), | |
| : m_expr(dil_expr), m_lexed_tokens(std::move(lexed_tokens)), m_tokens_idx(UINT_MAX), |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // "eof" token; to be returned by lexer when 'look ahead' fails. | |
| Token m_eof_token; | |
| }; | |
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| add_lldb_library(lldbValueObject | ||
| DILLexer.cpp | ||
| ValueObject.cpp | ||
| ValueObjectCast.cpp | ||
| ValueObjectChild.cpp | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,132 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| //===-- DILLexer.cpp ------------------------------------------------------===// | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // See https://llvm.org/LICENSE.txt for license information. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // This implements the recursive descent parser for the Data Inspection | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Language (DIL), and its helper functions, which will eventually underlie the | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // 'frame variable' command. The language that this parser recognizes is | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // described in lldb/docs/dil-expr-lang.ebnf | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| //===----------------------------------------------------------------------===// | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #include "lldb/ValueObject/DILLexer.h" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #include "lldb/Utility/Status.h" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #include "llvm/ADT/StringSwitch.h" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| namespace lldb_private::dil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| llvm::StringRef Token::GetTokenName(Kind kind) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| switch (kind) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| case Kind::coloncolon: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "coloncolon"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| case Kind::eof: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "eof"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| case Kind::identifier: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "identifier"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| case Kind::l_paren: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "l_paren"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| case Kind::r_paren: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "r_paren"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| case Kind::unknown: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "unknown"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| static bool IsLetter(char c) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return ('a' <= c && c <= 'z') || ('A' <= c && c <= 'Z'); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| static bool IsDigit(char c) { return '0' <= c && c <= '9'; } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // A word starts with a letter, underscore, or dollar sign, followed by | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // letters ('a'..'z','A'..'Z'), digits ('0'..'9'), and/or underscores. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| static std::optional<llvm::StringRef> IsWord(llvm::StringRef expr, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| llvm::StringRef &remainder) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| llvm::StringRef::iterator cur_pos = expr.end() - remainder.size(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| llvm::StringRef::iterator start = cur_pos; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| bool dollar_start = false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Must not start with a digit. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (cur_pos == expr.end() || IsDigit(*cur_pos)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return std::nullopt; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // First character *may* be a '$', for a register name or convenience | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
labath marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // variable. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (*cur_pos == '$') { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| dollar_start = true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ++cur_pos; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Contains only letters, digits or underscores | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (; cur_pos != expr.end(); ++cur_pos) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| char c = *cur_pos; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!IsLetter(c) && !IsDigit(c) && c != '_') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| break; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // If first char is '$', make sure there's at least one mare char, or it's | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // invalid. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (dollar_start && (cur_pos - start <= 1)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cur_pos = start; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return std::nullopt; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (cur_pos == start) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return std::nullopt; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| llvm::StringRef word = expr.substr(start - expr.begin(), cur_pos - start); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (remainder.consume_front(word)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return word; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return std::nullopt; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| llvm::StringRef::iterator cur_pos = expr.end() - remainder.size(); | |
| llvm::StringRef::iterator start = cur_pos; | |
| bool dollar_start = false; | |
| // Must not start with a digit. | |
| if (cur_pos == expr.end() || IsDigit(*cur_pos)) | |
| return std::nullopt; | |
| // First character *may* be a '$', for a register name or convenience | |
| // variable. | |
| if (*cur_pos == '$') { | |
| dollar_start = true; | |
| ++cur_pos; | |
| } | |
| // Contains only letters, digits or underscores | |
| for (; cur_pos != expr.end(); ++cur_pos) { | |
| char c = *cur_pos; | |
| if (!IsLetter(c) && !IsDigit(c) && c != '_') | |
| break; | |
| } | |
| // If first char is '$', make sure there's at least one mare char, or it's | |
| // invalid. | |
| if (dollar_start && (cur_pos - start <= 1)) { | |
| cur_pos = start; | |
| return std::nullopt; | |
| } | |
| if (cur_pos == start) | |
| return std::nullopt; | |
| llvm::StringRef word = expr.substr(start - expr.begin(), cur_pos - start); | |
| if (remainder.consume_front(word)) | |
| return word; | |
| return std::nullopt; | |
| const char *start = remainder.data(); | |
| remainder.consume_front("$"); // initial '$' is valid | |
| remainder = remainder.drop_while([](char c){ return IsDigit(c) || IsLetter(c) || c=='_'; }); | |
| llvm::StringRef candidate(start, remainder.data()-start); | |
| if (candidate.empty() || candidate == "$" || IsDigit(candidate[0])) | |
| return std::nullopt; | |
| return candidate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Not very surprisingly) I find it easier to understand my code than your code, so I would prefer to keep my code. But if you really want me to use something like your code instead I will ...
One question: When constructing 'candidate', you've already removed all the characters that should comprise the candidate word from remainder. Since those characters are no longer there, doesn't that make 'start' invalid? (It's pointing to something that's been removed from 'remainder'?). How can 'remainder.data() - start' possibly work at this point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The key thing to realize here is that a StringRef is just a fancy name for a pair<const char *, size_t>. On its own, it doesn't make anything valid. It's only valid as long as the string it points to is valid. And the same goes for the validity pointers you get from the StringRef. This is just a pointer to the parsed string, which will remain valid for as long as that string is around -- regardless of what happens to the StringRef.
That said, I realized a different problem with this code. It will end up modifying the remainder even in the failure case. So here's a slightly different version which avoids that. This one isn't completely equivalent, as I've dropped the requirement on the positioning of the dollar sign, but that's something I very strongly believe we should do anyway.
StringRef candidate = remainder.take_while([](char c) { return IsDigit(c) || IsLetter(c) || c=='_' || c=='$'; });
if (candidate.empty() || IsDigit(candidate[0]))
return std::nullopt;
remainder.drop_front(candidate.size());
return candidate;
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| llvm::StringRef::iterator cur_pos = expr.end() - remainder.size(); | |
| llvm::StringRef::iterator cur_pos = remainder.begin(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your suggestion is not correct. "cur_pos" is supposed to refer to the position of the currently-being-lexed word/token/etc in the original input string, not its position in remainder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that's the same thing. Both stringrefs are just pointers to the same underlying string. StringRef::iterator is just a fancy name for const char *. Would it look better to you if this was written as const char *cur_pos = remainder.data(); ?
Or even better, since this is only used constructing the position value below (I'm ignoring the check below), you can just drop this and construct position directly as remainder.data()-expr.data()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Both stringrefs are pointers to the same underlying string" -- I did not know that. SO...there's a string (const char *) in memory, that both StringRefs point to, and updating the StringRefs never updates the underlying memory contents? Hm...it would be nice if there was documentation somewhere that made this clear (the doxygen stuff does NOT). Thanks for the explanation & clarification.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (remainder.empty() || cur_pos == expr.end()) | |
| if (remainder.empty()) |
If the two conditions aren't equivalent, I'd like to know why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because they're checking on two different StringRefs? If this function has been called properly (with the correct arguments), then the two probablye should be equivalent. But I don't like to make assumptions like that without checking them...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I see code like this I start to wonder whether there's an edge case I'm missing. Defensive checks may make sense on some external API which is called from a bunch of places, but here we're talking about a private function that's only called from a single place. At best that deserves an assertion (you could e.g. say assert(expr.end() == remainder.end()) at the beginning of the function.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (maybe_word) { | |
| llvm::StringRef word = *maybe_word; | |
| return Token(Token::identifier, word.str(), position); | |
| } | |
| if (maybe_word) | |
| return Token(Token::identifier, maybe_word->str(), position); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(it's shorter, and I don't think the extra local variable helps anything)
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (remainder.consume_front(str)) { | |
| cur_pos += strlen(str); | |
| return Token(kind, str, position); | |
| } | |
| if (remainder.consume_front(str)) | |
| return Token(kind, str, position); |
(cur_pos isn't used after this)
Uh oh!
There was an error while loading. Please reload this page.