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

Report Unknown Doc-Comment Tags in Slice #3127

Merged
merged 9 commits into from
Nov 13, 2024
Merged

Report Unknown Doc-Comment Tags in Slice #3127

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c77f45c
Removed 'misc' comment tags, and issued warnings for unknown tags.
InsertCreativityHere Nov 11, 2024
45790f9
Added test and fixed a typo.
InsertCreativityHere Nov 11, 2024
8264851
Decided to use 'remarks' to pay homage.
InsertCreativityHere Nov 11, 2024
c7541e7
Added warning category to suppression logic.
InsertCreativityHere Nov 11, 2024
ed4b5c2
Tweaked some wording.
InsertCreativityHere Nov 12, 2024
88e52b8
Updated wording of error message.
InsertCreativityHere Nov 12, 2024
756a313
Made a message shorter to appease clang-format.
InsertCreativityHere Nov 12, 2024
60f110e
Updated other warning message.
InsertCreativityHere Nov 12, 2024
9b85f54
Added extra quote by accident.
InsertCreativityHere Nov 12, 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
81 changes: 45 additions & 36 deletions cpp/src/Slice/Parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,10 @@ Slice::DefinitionContext::initSuppressedWarnings()
{
_suppressedWarnings.insert(InvalidMetadata);
}
else if (s == "invalid-comment")
{
_suppressedWarnings.insert(InvalidComment);
}
else
{
ostringstream os;
Expand Down Expand Up @@ -320,12 +324,6 @@ Slice::Comment::overview() const
return _overview;
}

StringList
Slice::Comment::misc() const
{
return _misc;
}

StringList
Slice::Comment::seeAlso() const
{
Expand Down Expand Up @@ -777,95 +775,99 @@ Slice::Contained::parseComment(function<string(string, string)> linkFormatter, b

enum State
{
StateMisc,
StateUnknown,
StateParam,
StateThrows,
StateSee,
StateReturn,
StateDeprecated
};
State state = StateMisc;
State state = StateUnknown;
string name;
const string ws = " \t";
const string paramTag = "@param";
const string throwsTag = "@throws";
const string exceptionTag = "@exception";
const string seeTag = "@see";
const string returnTag = "@return";
const string deprecatedTag = "@deprecated";
const string seeTag = "@see";
for (; i != lines.end(); ++i)
{
const string l = IceInternal::trim(*i);
string line;
if (parseCommentLine(l, paramTag, true, name, line))
string lineText;
Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed to avoid shadowing the line method which is defined on this class.

if (parseCommentLine(l, paramTag, true, name, lineText))
{
if (!line.empty())
if (!lineText.empty())
{
state = StateParam;
StringList sl;
sl.push_back(line); // The first line of the description.
sl.push_back(lineText); // The first line of the description.
comment->_parameters[name] = sl;
}
}
else if (parseCommentLine(l, throwsTag, true, name, line))
else if (parseCommentLine(l, throwsTag, true, name, lineText))
{
if (!line.empty())
if (!lineText.empty())
{
state = StateThrows;
StringList sl;
sl.push_back(line); // The first line of the description.
sl.push_back(lineText); // The first line of the description.
comment->_exceptions[name] = sl;
}
}
else if (parseCommentLine(l, exceptionTag, true, name, line))
else if (parseCommentLine(l, exceptionTag, true, name, lineText))
{
if (!line.empty())
if (!lineText.empty())
{
state = StateThrows;
StringList sl;
sl.push_back(line); // The first line of the description.
sl.push_back(lineText); // The first line of the description.
comment->_exceptions[name] = sl;
}
}
else if (parseCommentLine(l, seeTag, false, name, line))
else if (parseCommentLine(l, seeTag, false, name, lineText))
{
if (!line.empty())
if (!lineText.empty())
{
state = StateMisc; // Reset the state; `@see` cannot span multiple lines.
comment->_seeAlso.push_back(line);
state = StateSee;
comment->_seeAlso.push_back(lineText);
}
}
else if (parseCommentLine(l, returnTag, false, name, line))
else if (parseCommentLine(l, returnTag, false, name, lineText))
{
if (!line.empty())
if (!lineText.empty())
{
state = StateReturn;
comment->_returns.push_back(line); // The first line of the description.
comment->_returns.push_back(lineText); // The first line of the description.
}
}
else if (parseCommentLine(l, deprecatedTag, false, name, line))
else if (parseCommentLine(l, deprecatedTag, false, name, lineText))
{
comment->_isDeprecated = true;
if (!line.empty())
if (!lineText.empty())
{
state = StateDeprecated;
comment->_deprecated.push_back(line); // The first line of the description.
comment->_deprecated.push_back(lineText); // The first line of the description.
}
}
else if (!l.empty())
{
if (l[0] == '@')
{
//
// Treat all other tags as miscellaneous comments.
//
state = StateMisc;
// We've encountered an unknown doc tag.
auto unknownTag = l.substr(0, l.find_first_of(" \t:"));
string msg = "ignoring unknown doc tag '" + unknownTag + "' in comment";
unit()->warning(file(), line(), InvalidComment, msg);
state = StateUnknown;
continue;
}

switch (state)
{
case StateMisc:
case StateUnknown:
{
comment->_misc.push_back(l);
// Getting here means we've hit an unknown tag that spanned multiple lines.
// We immediately break - ignoring and discarding the line.
break;
}
case StateParam:
Expand All @@ -892,6 +894,14 @@ Slice::Contained::parseComment(function<string(string, string)> linkFormatter, b
comment->_exceptions[name] = sl;
break;
}
case StateSee:
{
// This isn't allowed - '@see' tags cannot span multiple lines. We've already kept the original
// line by this point, but we ignore any lines that follow it (until hitting another '@' tag).
string msg = "'@see' tags cannot span multiple lines and must be of the form: '@see identifier'";
unit()->warning(file(), line(), InvalidComment, msg);
break;
}
case StateReturn:
{
comment->_returns.push_back(l);
Expand All @@ -908,7 +918,6 @@ Slice::Contained::parseComment(function<string(string, string)> linkFormatter, b

trimLines(comment->_overview);
trimLines(comment->_deprecated);
trimLines(comment->_misc);
trimLines(comment->_returns);

return comment;
Expand Down
8 changes: 3 additions & 5 deletions cpp/src/Slice/Parser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ namespace Slice
{
All,
Deprecated,
InvalidMetadata
InvalidMetadata,
InvalidComment
Copy link
Member

Choose a reason for hiding this comment

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

Should be InvalidDocComment. A plain comment is always valid and ignored.

};

class GrammarBase;
Expand Down Expand Up @@ -252,9 +253,7 @@ namespace Slice

/// Contains all introductory lines up to the first tag.
StringList overview() const;
/// Contains unrecognized tags.
StringList misc() const;
/// Targets of @see tags.
/// Targets of '@see' tags.
Copy link
Member

Choose a reason for hiding this comment

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

We should rename this class DocComment.

StringList seeAlso() const;

/// Description of an operation's return value.
Expand All @@ -270,7 +269,6 @@ namespace Slice
bool _isDeprecated;
StringList _deprecated;
StringList _overview;
StringList _misc;
StringList _seeAlso;

StringList _returns;
Expand Down
11 changes: 0 additions & 11 deletions cpp/src/slice2cpp/Gen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,11 +398,6 @@ namespace
writeDocLines(out, doc->overview(), true);
}

if (!doc->misc().empty())
{
writeDocLines(out, doc->misc(), true);
}

if (!doc->seeAlso().empty())
{
writeSeeAlso(out, doc->seeAlso());
Expand Down Expand Up @@ -541,12 +536,6 @@ namespace
writeOpDocExceptions(out, op, doc);
}

const auto& misc = doc->misc();
if (!misc.empty())
{
writeDocLines(out, misc, true);
}

const auto& seeAlso = doc->seeAlso();
if (!seeAlso.empty())
{
Expand Down
18 changes: 0 additions & 18 deletions cpp/src/slice2java/Gen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1860,12 +1860,6 @@ Slice::JavaVisitor::writeDocComment(Output& out, const UnitPtr& unt, const Comme
writeDocCommentLines(out, dc->overview());
}

if (!dc->misc().empty())
{
out << nl << " * ";
writeDocCommentLines(out, dc->misc());
}

if (!dc->seeAlso().empty())
{
out << nl << " *";
Expand Down Expand Up @@ -2007,12 +2001,6 @@ Slice::JavaVisitor::writeProxyDocComment(
}
}

if (!dc->misc().empty())
{
out << nl << " * ";
writeDocCommentLines(out, dc->misc());
}

if (!dc->seeAlso().empty())
{
out << nl << " *";
Expand Down Expand Up @@ -2172,12 +2160,6 @@ Slice::JavaVisitor::writeServantDocComment(
}
}

if (!dc->misc().empty())
{
out << nl << " * ";
writeDocCommentLines(out, dc->misc());
}

if (!dc->seeAlso().empty())
{
out << nl << " *";
Expand Down
10 changes: 0 additions & 10 deletions cpp/src/slice2js/Gen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,11 +495,6 @@ Slice::JsVisitor::writeDocCommentFor(const ContainedPtr& p, bool includeDeprecat
writeDocLines(_out, comment->overview(), true);
}

if (!comment->misc().empty())
{
writeDocLines(_out, comment->misc(), true);
}

if (!comment->seeAlso().empty())
{
writeSeeAlso(_out, comment->seeAlso());
Expand Down Expand Up @@ -2504,11 +2499,6 @@ Slice::Gen::TypeScriptVisitor::writeOpDocSummary(
{
writeOpDocExceptions(out, op, comment);

if (!comment->misc().empty())
{
writeDocLines(out, comment->misc(), true);
}

if (!comment->seeAlso().empty())
{
writeSeeAlso(out, comment->seeAlso());
Expand Down
28 changes: 0 additions & 28 deletions cpp/src/slice2matlab/Main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -970,13 +970,6 @@ namespace
}
}

StringList docMisc = doc->misc();
if (!docMisc.empty())
{
out << nl << "%";
writeDocLines(out, docMisc, true);
}

StringList docSeeAlso = doc->seeAlso();
if (!docSeeAlso.empty())
{
Expand Down Expand Up @@ -1138,13 +1131,6 @@ namespace
}
}

StringList docMisc = doc->misc();
if (!docMisc.empty())
{
out << nl << "%";
writeDocLines(out, docMisc, true);
}

StringList docSeeAlso = doc->seeAlso();
if (!docSeeAlso.empty())
{
Expand Down Expand Up @@ -1224,13 +1210,6 @@ namespace
out << nl << "% checkedCast - Contacts the remote server to verify that the object implements this type.";
out << nl << "% uncheckedCast - Downcasts the given proxy to this type without contacting the remote server.";

StringList docMisc = doc->misc();
if (!docMisc.empty())
{
out << nl << "%";
writeDocLines(out, docMisc, true);
}

StringList docSeeAlso = doc->seeAlso();
if (!docSeeAlso.empty())
{
Expand Down Expand Up @@ -1263,7 +1242,6 @@ namespace
}

StringList docOverview = doc->overview();
StringList docMisc = doc->misc();
StringList docSeeAlso = doc->seeAlso();
StringList docDeprecated = doc->deprecated();
bool docIsDeprecated = doc->isDeprecated();
Expand All @@ -1287,12 +1265,6 @@ namespace
writeDocLines(out, docOverview, false);
}

if (!docMisc.empty())
{
out << nl << "%";
writeDocLines(out, docMisc, true);
}

if (!docSeeAlso.empty())
{
out << nl << "%";
Expand Down
Loading
Loading