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

Jump to bottom

Centralize comment parsing #3030

Merged
merged 11 commits into from
Nov 5, 2024
Merged

Centralize comment parsing #3030

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4bb8fb1
Centralize comment-parsing code.
InsertCreativityHere Oct 31, 2024
5d1740c
Remove unused parameters.
InsertCreativityHere Oct 31, 2024
06a9687
Not sure why it built locally without 'include functional' but whatev…
InsertCreativityHere Oct 31, 2024
eb2d8ca
Some platforms want us to qualify the 'replace' function.
InsertCreativityHere Oct 31, 2024
88a1aee
Pull in the algorithm header. Still concerning this worked locally.
InsertCreativityHere Oct 31, 2024
0947315
Removed logic for injecting deprecated metadata into doc comments.
InsertCreativityHere Oct 31, 2024
02c384b
Formatting...
InsertCreativityHere Oct 31, 2024
3609630
Formatting... again...
InsertCreativityHere Oct 31, 2024
8a9ec95
Fixed first round of review comments.
InsertCreativityHere Nov 4, 2024
f5a69e3
Tweaked comment line splitting logic.
InsertCreativityHere Nov 4, 2024
d97b2b9
More review comment fixes.
InsertCreativityHere Nov 5, 2024
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
104 changes: 50 additions & 54 deletions cpp/src/Slice/Parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -639,14 +639,15 @@ namespace
}
}

StringList splitComment(const string& c, bool stripMarkup)
StringList splitComment(string_view c, function<string(string)> linkFormatter, bool stripMarkup)
{
string comment = c;
string comment = string{c};
string::size_type pos;

if (stripMarkup)
{
// Strip HTML markup and javadoc links.
string::size_type pos = 0;
// Strip HTML markup.
pos = 0;
Copy link
Member

Choose a reason for hiding this comment

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

move to line 644

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

do
{
pos = comment.find('<', pos);
Expand All @@ -660,52 +661,38 @@ namespace
comment.erase(pos, endpos - pos + 1);
}
} while (pos != string::npos);
}

const string link = "{@link";
pos = 0;
do
// Fix any javadoc using the provided link formatter.
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this comment be about Slice links, not javadoc links. I guess we have this comment because Slice doc comments are based on javadoc, still this is about parsing the Slice doc comment link.

Copy link
Member Author

Choose a reason for hiding this comment

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

I edited the comment to talk about Slice instead of javadoc.

const string link = "{@link ";
Copy link
Member Author

Choose a reason for hiding this comment

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

There should always be a space after the @link.
This code would let through things like {@linkStringSeq} which is bogus.

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this update break the link if you use \t or \n after {@link. A corner case...

Copy link
Member Author

Choose a reason for hiding this comment

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

@link cannot span multiple lines, so \n shouldn't be a problem.
But, you're right that \t used to work and won't anymore. But I'm pretty sure it only worked by accident.
Using \t here would be putting tabs between the words in a sentence. It would work, but it's a very strange thing to do. I'm not convinced any sane person is doing this.

pos = 0;
do
Copy link
Member

Choose a reason for hiding this comment

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

This do/while loop with the inner if checking the same condition, can be refactor into a single while loop.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, totally right.
Fixed.

{
pos = comment.find(link, pos);
if (pos != string::npos)
{
pos = comment.find(link, pos);
if (pos != string::npos)
string::size_type endpos = comment.find('}', pos);
if (endpos != string::npos)
{
comment.erase(pos, link.size() + 1); // Erase trailing white space too.
string::size_type endpos = comment.find('}', pos);
if (endpos != string::npos)
{
string ident = comment.substr(pos, endpos - pos);
comment.erase(pos, endpos - pos + 1);

//
// Replace links of the form {@link Type#member} with "Type.member".
//
string::size_type hash = ident.find('#');
string rest;
if (hash != string::npos)
{
rest = ident.substr(hash + 1);
ident = ident.substr(0, hash);
if (!ident.empty())
{
if (!rest.empty())
{
ident += "." + rest;
}
}
else if (!rest.empty())
{
ident = rest;
}
}

comment.insert(pos, ident);
}
// Extract the linked to identifier.
string::size_type identStart = comment.find_first_not_of(" \t", pos + link.size());
string::size_type identEnd = comment.find_last_not_of(" \t", endpos) + 1;
string ident = comment.substr(identStart, identEnd - identStart);

// Then erase the entire '{@link foo}' tag from the comment.
comment.erase(pos, endpos - pos + 1);

// In it's place, insert the correctly formatted link.
string formattedLink = linkFormatter(ident);
comment.insert(pos, formattedLink);
pos += formattedLink.length();
}
} while (pos != string::npos);
}
}
} while (pos != string::npos);

StringList result;

string::size_type pos = 0;
pos = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to add a comment about the lines below. I think we can also slightly simplify it, we don't need special logic for the last line if we are calling trimLines.

We can also update the calls to IceInternal::trim to pass a string_view, trim parameter is now a string_view.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a comment and simplified the way we handled the last line of text.

string::size_type nextPos;
while ((nextPos = comment.find_first_of('\n', pos)) != string::npos)
{
Expand Down Expand Up @@ -770,23 +757,31 @@ namespace
}

CommentPtr
Slice::Contained::parseComment(bool stripMarkup) const
Slice::Contained::parseComment(
function<string(string)> linkFormatter,
bool includeDeprecatedMetadata,
bool stripMarkup) const
{
return parseComment(_comment, stripMarkup);
}
CommentPtr comment = make_shared<Comment>();

CommentPtr
Slice::Contained::parseComment(const string& text, bool stripMarkup) const
{
if (text.empty())
// Check for deprecated metadata and add it to the comment if necessary.
Copy link
Member

@bernardnormier bernardnormier Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Please don't do that. I removed this behavior in #2970, and looking at this PR, it appears I forgot do removed it from Swift and MATLAB. Please fixes these too.

Copy link
Member Author

Choose a reason for hiding this comment

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

In fairness, the comment code for Swift and MATLAB was completely separate from all the other comment code, so it's not surprising.

I removed all the includeDeprecated stuff, and am happy to see it go!

if (includeDeprecatedMetadata)
{
return nullptr;
comment->_isDeprecated = isDeprecated();
if (auto reason = getDeprecationReason())
{
comment->_deprecated.push_back(IceInternal::trim(*reason));
}
}

// Split up the comment text into lines.
CommentPtr comment = make_shared<Comment>();
StringList lines = splitComment(text, stripMarkup);
// Split the comment's raw text up into lines.
StringList lines = splitComment(_comment, linkFormatter, stripMarkup);
Copy link
Member

Choose a reason for hiding this comment

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

The linkFormater is not used here, we can move it to splitComment.

Suggested change
StringList lines = splitComment(_comment, linkFormatter, stripMarkup);
StringList lines = splitComment(_comment, std::move(linkFormatter), stripMarkup);

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I guess there's no reason not to!
Fixed.

if (lines.empty() && !comment->_isDeprecated)
{
return nullptr;
}

// Parse the comment's text.
StringList::const_iterator i;
for (i = lines.begin(); i != lines.end(); ++i)
{
Expand Down Expand Up @@ -853,6 +848,7 @@ Slice::Contained::parseComment(const string& text, bool stripMarkup) const
{
if (!line.empty())
{
state = StateMisc; // Reset the state; `@see` cannot span multiple lines.
Copy link
Member Author

Choose a reason for hiding this comment

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

Fixes a bug where we'd sometimes generate multi-line see statements.
These should always be a single line like: @see MyType.op79

comment->_seeAlso.push_back(line);
}
}
Expand Down
6 changes: 4 additions & 2 deletions cpp/src/Slice/Parser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,8 +375,10 @@ namespace Slice
int line() const;

std::string comment() const;
CommentPtr parseComment(bool stripMarkup) const;
CommentPtr parseComment(const std::string& text, bool stripMarkup) const;
Copy link
Member Author

Choose a reason for hiding this comment

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

We only need one function, the 2nd one was a hack-around to let ice2slice massage it's links before parsing. The new linkFormatter approach solves this problem though.

CommentPtr parseComment(
std::function<std::string(std::string)> linkFormatter,
bool includeDeprecatedMetadata = false,
Copy link
Member

Choose a reason for hiding this comment

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

See comment in .cpp

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

bool stripMarkup = false) const;

int includeLevel() const;

Expand Down
9 changes: 8 additions & 1 deletion cpp/src/ice2slice/Gen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,9 +272,16 @@ namespace
return os.str();
}

string slice2LinkFormatter(string identifier)
{
// Replace links of the form `{@link Type#member}` with `{@link Type.member}`.
replace(identifier.begin(), identifier.end(), '#', '.');
return "{@link " + identifier + "}";
}

void writeComment(const ContainedPtr& contained, Output& out)
{
CommentPtr comment = contained->parseComment(true);
CommentPtr comment = contained->parseComment(slice2LinkFormatter, false, true);
if (!comment)
{
return;
Expand Down
18 changes: 11 additions & 7 deletions cpp/src/slice2cpp/Gen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,12 @@ namespace
return r;
}

/// Returns a doxygen formatted link to the provided Slice identifier.
string cppLinkFormatter(string identifier)
{
return "{@link " + fixKwd(identifier) + "}";
Copy link
Member Author

Choose a reason for hiding this comment

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

Apparently we weren't escaping names in C++ links...
Linking to an identifier that is a C++ keyword is a very niche case, but still good to fix.

}

void writeDocLines(Output& out, const StringList& lines, bool commentFirst, const string& space = " ")
{
auto l = lines.cbegin();
Expand Down Expand Up @@ -374,7 +380,7 @@ namespace
return;
}

CommentPtr doc = p->parseComment(false);
CommentPtr doc = p->parseComment(cppLinkFormatter);

out << nl << "/**";

Expand Down Expand Up @@ -1689,7 +1695,7 @@ Slice::Gen::ProxyVisitor::visitOperation(const OperationPtr& p)

const string deprecatedSymbol = getDeprecatedSymbol(p);

CommentPtr comment = p->parseComment(false);
CommentPtr comment = p->parseComment(cppLinkFormatter);
const string contextDoc = "@param " + contextParam + " The Context map to send with the invocation.";
const string futureDoc = "The future object for the invocation.";

Expand Down Expand Up @@ -2115,7 +2121,7 @@ Slice::Gen::DataDefVisitor::visitExceptionStart(const ExceptionPtr& p)
typeToString(dataMember->type(), dataMember->optional(), scope, dataMember->getMetadata(), _useWstring);
allParamDecls.push_back(typeName + " " + fixKwd(dataMember->name()));

CommentPtr comment = dataMember->parseComment(false);
CommentPtr comment = dataMember->parseComment(cppLinkFormatter);
if (comment)
{
allComments[dataMember->name()] = comment;
Expand Down Expand Up @@ -2592,15 +2598,13 @@ Slice::Gen::DataDefVisitor::emitOneShotConstructor(const ClassDefPtr& p)
string typeName =
typeToString(dataMember->type(), dataMember->optional(), scope, dataMember->getMetadata(), _useWstring);
allParamDecls.push_back(typeName + " " + fixKwd(dataMember->name()));
CommentPtr comment = dataMember->parseComment(false);
CommentPtr comment = dataMember->parseComment(cppLinkFormatter);
if (comment)
{
allComments[dataMember->name()] = comment;
}
}

CommentPtr comment = p->parseComment(false);

H << sp;
H << nl << "/**";
H << nl << " * One-shot constructor to initialize all data members.";
Expand Down Expand Up @@ -2950,7 +2954,7 @@ Slice::Gen::InterfaceVisitor::visitOperation(const OperationPtr& p)
const string currentTypeDecl = "const " + getUnqualified("::Ice::Current&", interfaceScope);
const string currentDecl = currentTypeDecl + " " + currentParam;

CommentPtr comment = p->parseComment(false);
CommentPtr comment = p->parseComment(cppLinkFormatter);

if (ret)
{
Expand Down
Loading
Loading