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 10 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
109 changes: 47 additions & 62 deletions cpp/src/Slice/Parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
#include <algorithm>
#include <cassert>
#include <cstring>
#include <functional>
#include <iterator>
#include <limits>

Expand Down Expand Up @@ -639,14 +638,13 @@ namespace
}
}

StringList splitComment(const string& c, bool stripMarkup)
StringList splitComment(string comment, function<string(string, string)> linkFormatter, bool stripMarkup)
{
string comment = c;
string::size_type 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.

Isn't a string preferred in this case, so that we can use std::move and avoid the copy when the parameter is an rvalue reference?

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 switched it to take a string, since it's a simpler type.
But, this helper function is only called from one place, which will never use move semantics.


if (stripMarkup)
{
// Strip HTML markup and javadoc links.
string::size_type pos = 0;
// Strip HTML markup.
do
{
pos = comment.find('<', pos);
Expand All @@ -660,64 +658,55 @@ namespace
comment.erase(pos, endpos - pos + 1);
}
} while (pos != string::npos);
}

const string link = "{@link";
pos = 0;
do
// Fix any link tags using the provided link formatter.
const string link = "{@link ";
pos = comment.find(link, pos);
while (pos != string::npos)
{
string::size_type endpos = comment.find('}', pos);
if (endpos != string::npos)
{
pos = comment.find(link, pos);
if (pos != string::npos)
// 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);

// Split the link into 'class' and 'member' components (links are of the form 'class#member').
string memberComponent;
string::size_type hashPos = ident.find('#');
if (hashPos != 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);
}
memberComponent = ident.substr(hashPos + 1);
ident.erase(hashPos);
}
} while (pos != string::npos);
else
{
memberComponent = "";
}
Copy link
Member

Choose a reason for hiding this comment

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

Seems this else is not required, memberComponent is already "" in this 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.

Removed!


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

// Split the comment into separate lines, and removing any trailing whitespace and lines from it.
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)
{
result.push_back(IceInternal::trim(string(comment, pos, nextPos - pos)));
result.push_back(IceInternal::trim(comment.substr(pos, nextPos - pos)));
pos = nextPos + 1;
}
string lastLine = IceInternal::trim(string(comment, pos));
if (!lastLine.empty())
{
result.push_back(lastLine);
}

result.push_back(IceInternal::trim(comment.substr(pos)));
trimLines(result);

return result;
Expand Down Expand Up @@ -770,23 +759,18 @@ namespace
}

CommentPtr
Slice::Contained::parseComment(bool stripMarkup) const
Slice::Contained::parseComment(function<string(string, string)> linkFormatter, 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())
// Split the comment's raw text up into lines.
StringList lines = splitComment(_comment, std::move(linkFormatter), stripMarkup);
if (lines.empty() && !comment->_isDeprecated)
{
return nullptr;
}

// Split up the comment text into lines.
CommentPtr comment = make_shared<Comment>();
StringList lines = splitComment(text, stripMarkup);

// Parse the comment's text.
StringList::const_iterator i;
for (i = lines.begin(); i != lines.end(); ++i)
{
Expand Down Expand Up @@ -853,6 +837,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 @@ -5,6 +5,7 @@

#include <array>
#include <cstdint>
#include <functional>
#include <list>
#include <map>
#include <memory>
Expand Down Expand Up @@ -375,8 +376,9 @@ 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, std::string)> linkFormatter,
bool stripMarkup = false) const;

int includeLevel() const;

Expand Down
22 changes: 21 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 @@ -4,6 +4,7 @@

#include "Gen.h"

#include <algorithm>
#include <cassert>

using namespace std;
Expand Down Expand Up @@ -272,9 +273,28 @@ namespace
return os.str();
}

string slice2LinkFormatter(string identifier, string memberComponent)
{
// Replace links of the form `{@link Type#member}` with `{@link Type.member}`.
string result = "{@link ";
if (memberComponent.empty())
{
result += identifier;
}
else if (identifier.empty())
{
result += memberComponent;
}
else
{
result += identifier + "." + memberComponent;
}
return result += "}";
}

void writeComment(const ContainedPtr& contained, Output& out)
{
CommentPtr comment = contained->parseComment(true);
CommentPtr comment = contained->parseComment(slice2LinkFormatter, true);
if (!comment)
{
return;
Expand Down
1 change: 0 additions & 1 deletion cpp/src/slice2cpp/CPlusPlusUtil.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
#include <algorithm>
#include <cassert>
#include <cstring>
#include <functional>

#ifndef _WIN32
# include <fcntl.h>
Expand Down
27 changes: 20 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,21 @@ namespace
return r;
}

/// Returns a doxygen formatted link to the provided Slice identifier.
string cppLinkFormatter(string identifier, string memberComponent)
{
string result = "{@link ";
if (!identifier.empty())
{
result += fixKwd(identifier);
}
if (!memberComponent.empty())
{
result += "#" + fixKwd(memberComponent);
}
return result + "}";
}

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

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

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

Expand Down Expand Up @@ -1689,7 +1704,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 +2130,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 +2607,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 +2963,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