-
Notifications
You must be signed in to change notification settings - Fork 13.5k
[Clang][Comments] Add argument parsing for @throw @throws @exception #84726
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 all commits
49fd778
7da378b
ea7b933
b1c73a1
4432d32
421f585
9eb681d
0684ecf
0e56ade
6b165de
3131f80
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 |
---|---|---|
|
@@ -89,6 +89,31 @@ class TextTokenRetokenizer { | |
} | ||
} | ||
|
||
/// Extract a template type | ||
bool lexTemplate(SmallString<32> &WordText) { | ||
unsigned BracketCount = 0; | ||
while (!isEnd()) { | ||
const char C = peek(); | ||
WordText.push_back(C); | ||
consumeChar(); | ||
Comment on lines
+96
to
+98
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be nice to modify There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer to do that as a separate refactor PR, I think it's outside the scope of this change. |
||
switch (C) { | ||
case '<': { | ||
BracketCount++; | ||
break; | ||
} | ||
case '>': { | ||
BracketCount--; | ||
if (!BracketCount) | ||
return true; | ||
break; | ||
} | ||
default: | ||
break; | ||
} | ||
} | ||
return false; | ||
} | ||
|
||
/// Add a token. | ||
/// Returns true on success, false if there are no interesting tokens to | ||
/// fetch from lexer. | ||
|
@@ -149,6 +174,54 @@ class TextTokenRetokenizer { | |
addToken(); | ||
} | ||
|
||
/// Extract a type argument | ||
bool lexType(Token &Tok) { | ||
if (isEnd()) | ||
return false; | ||
|
||
// Save current position in case we need to rollback because the type is | ||
// empty. | ||
Position SavedPos = Pos; | ||
|
||
// Consume any leading whitespace. | ||
consumeWhitespace(); | ||
SmallString<32> WordText; | ||
const char *WordBegin = Pos.BufferPtr; | ||
SourceLocation Loc = getSourceLocation(); | ||
|
||
while (!isEnd()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some comments wouldn't hurt... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added as part of the latest commit, along with some light refactoring to make it more readable. No doubt it is a complex piece of code |
||
const char C = peek(); | ||
// For non-whitespace characters we check if it's a template or otherwise | ||
// continue reading the text into a word. | ||
if (!isWhitespace(C)) { | ||
if (C == '<') { | ||
if (!lexTemplate(WordText)) | ||
return false; | ||
} else { | ||
WordText.push_back(C); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this treat any non-whitespace character (other than There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As the code stands now, yes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which I think is fine, we are just trying to find a boundary |
||
consumeChar(); | ||
} | ||
} else { | ||
consumeChar(); | ||
break; | ||
} | ||
} | ||
|
||
const unsigned Length = WordText.size(); | ||
if (Length == 0) { | ||
Pos = SavedPos; | ||
return false; | ||
} | ||
|
||
char *TextPtr = Allocator.Allocate<char>(Length + 1); | ||
|
||
memcpy(TextPtr, WordText.c_str(), Length + 1); | ||
StringRef Text = StringRef(TextPtr, Length); | ||
|
||
formTokenWithChars(Tok, Loc, WordBegin, Length, Text); | ||
Comment on lines
+216
to
+221
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd like to see that extracted in a separate function (and replace the few places where we do the exact same thing) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As above, I would prefer to do that as a separate refactor PR, I think it's outside the scope of this change. |
||
return true; | ||
} | ||
|
||
/// Extract a word -- sequence of non-whitespace characters. | ||
bool lexWord(Token &Tok) { | ||
if (isEnd()) | ||
|
@@ -304,6 +377,23 @@ Parser::parseCommandArgs(TextTokenRetokenizer &Retokenizer, unsigned NumArgs) { | |
return llvm::ArrayRef(Args, ParsedArgs); | ||
} | ||
|
||
ArrayRef<Comment::Argument> | ||
Parser::parseThrowCommandArgs(TextTokenRetokenizer &Retokenizer, | ||
unsigned NumArgs) { | ||
auto *Args = new (Allocator.Allocate<Comment::Argument>(NumArgs)) | ||
Comment::Argument[NumArgs]; | ||
unsigned ParsedArgs = 0; | ||
Token Arg; | ||
|
||
while (ParsedArgs < NumArgs && Retokenizer.lexType(Arg)) { | ||
Args[ParsedArgs] = Comment::Argument{ | ||
SourceRange(Arg.getLocation(), Arg.getEndLocation()), Arg.getText()}; | ||
ParsedArgs++; | ||
} | ||
|
||
return llvm::ArrayRef(Args, ParsedArgs); | ||
} | ||
|
||
BlockCommandComment *Parser::parseBlockCommand() { | ||
assert(Tok.is(tok::backslash_command) || Tok.is(tok::at_command)); | ||
|
||
|
@@ -356,6 +446,9 @@ BlockCommandComment *Parser::parseBlockCommand() { | |
parseParamCommandArgs(PC, Retokenizer); | ||
else if (TPC) | ||
parseTParamCommandArgs(TPC, Retokenizer); | ||
else if (Info->IsThrowsCommand) | ||
S.actOnBlockCommandArgs( | ||
BC, parseThrowCommandArgs(Retokenizer, Info->NumArgs)); | ||
else | ||
S.actOnBlockCommandArgs(BC, parseCommandArgs(Retokenizer, Info->NumArgs)); | ||
|
||
|
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.
This should work okay for simple (most) cases, but we should have tests showing the boundaries, e.g.,
Foo<(1 > 0)>
or a typo likeFoo<Bar<T>
etc.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.
Added some more test cases that check this edge case behavior, like unequal numbers of opening and closing brackets as well as some other unusual edge cases.