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

Cleanup Deprecated Metadata #2129

Merged
merged 8 commits into from
May 10, 2024
Merged

Cleanup Deprecated Metadata #2129

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d7be95f
Fix bogus deprecated message in 'Glacier2/SessionHelper.h'.
InsertCreativityHere May 6, 2024
86785f6
Make this metadata in the switch tests look less weird.
InsertCreativityHere May 6, 2024
0e938cf
Fix typo in random js file.
InsertCreativityHere May 6, 2024
61310cd
Cleanup 'deprecated' metadata and message emission.
InsertCreativityHere May 7, 2024
893e403
Appease the tooling.
InsertCreativityHere May 7, 2024
abf24c3
Review fixes.
InsertCreativityHere May 8, 2024
b643b6e
Fixed a bool
InsertCreativityHere May 8, 2024
32d4ccd
Review comments.
InsertCreativityHere May 10, 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
4 changes: 2 additions & 2 deletions cpp/include/Glacier2/SessionHelper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,10 +181,10 @@ namespace Glacier2
std::string getRouterHost() const;

/// \cond INTERNAL
[[deprecated("is deprecated, use SessionFactoryHelper::setProtocol instead")]]
[[deprecated("use SessionFactoryHelper::setProtocol instead")]]
void setSecure(bool);

[[deprecated("is deprecated, use SessionFactoryHelper::getProtocol instead")]]
[[deprecated("use SessionFactoryHelper::getProtocol instead")]]
bool getSecure() const;
/// \endcond

Expand Down
36 changes: 28 additions & 8 deletions cpp/src/Slice/Parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -748,19 +748,14 @@ Slice::Contained::parseComment(bool stripMarkup) const
{
CommentPtr comment = make_shared<Comment>();

comment->_isDeprecated = false;
comment->_isDeprecated = isDeprecated(false);

//
// First check metadata for a deprecated tag.
//
string deprecateMetadata;
if (findMetaData("deprecate", deprecateMetadata))
if (auto reason = getDeprecateReason(false))
{
comment->_isDeprecated = true;
if (deprecateMetadata.find("deprecate:") == 0 && deprecateMetadata.size() > 10)
{
comment->_deprecated.push_back(IceUtilInternal::trim(deprecateMetadata.substr(10)));
}
comment->_deprecated.push_back(IceUtilInternal::trim(*reason));
}

if (!comment->_isDeprecated && _comment.empty())
Expand Down Expand Up @@ -1003,6 +998,31 @@ Slice::Contained::parseFormatMetaData(const list<string>& metaData)
return result;
}

bool
Slice::Contained::isDeprecated(bool checkParent) const
{
const string deprecate = "deprecate";
Copy link
Member

Choose a reason for hiding this comment

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

Even though your functions are named "isDeprecated", you didn't add "deprecated" as an alias for the "deprecate" metadata?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but only because isDeprecate isn't a sensible thing to say IMO. Nothing to do with our eventual renaming.
I plan to add the alias in my next PR, alongside removing the dispatch side.

Copy link
Member

Choose a reason for hiding this comment

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

I would be nice to update findMetaData to take a string_view and switch this const to a constexpr.

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'll do that in a follow-up PR though.
I want to avoid going too far down that rabbit hole though.
It'd probably take months to clean and modernize the compilers...

string metadata;
ContainedPtr parent = checkParent ? dynamic_pointer_cast<Contained>(_container) : nullptr;

return (findMetaData(deprecate, metadata) || (parent && parent->findMetaData(deprecate, metadata)));
}

optional<string>
Slice::Contained::getDeprecateReason(bool checkParent) const
{
const string prefix = "deprecate:";
string metadata;
ContainedPtr parent = checkParent ? dynamic_pointer_cast<Contained>(_container) : nullptr;

if (findMetaData(prefix, metadata) || (parent && parent->findMetaData(prefix, metadata)))
{
assert(metadata.find(prefix) == 0);
return metadata.substr(prefix.size());
}
return nullopt;
}

Slice::Contained::Contained(const ContainerPtr& container, const string& name)
: SyntaxTreeBase(container->unit()),
_container(container),
Expand Down
12 changes: 12 additions & 0 deletions cpp/src/Slice/Parser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,6 +397,18 @@ namespace Slice

static FormatType parseFormatMetaData(const std::list<std::string>&);

/// Returns true if this item is deprecated (due to the presence of `deprecate` metadata).
/// @param checkParent If true, this item's immediate container will also be checked for `deprecate` metadata.
/// @return True if this item (or possibly its container) has `deprecate` metadata on it, false otherwise.
bool isDeprecated(bool checkParent) const;

/// If this item is deprecated, return its deprecation message (if present).
bernardnormier marked this conversation as resolved.
Show resolved Hide resolved
/// This is the string argument that can be optionally provided with `deprecate` metadata.
/// @param checkParent If true, this item's immediate container will also be checked for `deprecate` messages.
/// @return The message provided to the `deprecate` metadata, if present. If `checkParent` is true, and both
/// this item and its parent have 'deprecate' messages, the item's message is returned, not its container's.
std::optional<std::string> getDeprecateReason(bool checkParent) const;

enum ContainedType
{
ContainedTypeSequence,
Expand Down
21 changes: 11 additions & 10 deletions cpp/src/slice2cpp/Gen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,18 +72,19 @@ namespace
}
}

string getDeprecateSymbol(const ContainedPtr& p1, const ContainedPtr& p2)
string getDeprecateSymbol(const ContainedPtr& p1)
{
string deprecateMetadata, deprecateSymbol;
if (p1->findMetaData("deprecate", deprecateMetadata) ||
(p2 != 0 && p2->findMetaData("deprecate", deprecateMetadata)))
string deprecateSymbol;
Copy link
Member

Choose a reason for hiding this comment

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

I'd use "deprecated" instead of "deprecate".

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. deprecateSymbol was just the old name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also fixed!

if (p1->isDeprecated(true))
{
string msg = "is deprecated";
if (deprecateMetadata.find("deprecate:") == 0 && deprecateMetadata.size() > 10)
if (auto reason = p1->getDeprecateReason(false))
{
msg = deprecateMetadata.substr(10);
deprecateSymbol = "[[deprecated(\"" + *reason + "\")]] ";
}
else
{
deprecateSymbol = "[[deprecated]] ";
}
deprecateSymbol = "[[deprecated(\"" + msg + "\")]] ";
}
return deprecateSymbol;
}
Expand Down Expand Up @@ -1701,7 +1702,7 @@ Slice::Gen::ProxyVisitor::visitOperation(const OperationPtr& p)
string futureTAbsolute = createOutgoingAsyncTypeParam(createOutgoingAsyncParams(p, "", _useWstring));
string lambdaT = createOutgoingAsyncTypeParam(lambdaOutParams);

const string deprecateSymbol = getDeprecateSymbol(p, interface);
const string deprecateSymbol = getDeprecateSymbol(p);

CommentPtr comment = p->parseComment(false);
const string contextDoc = "@param " + contextParam + " The Context map to send with the invocation.";
Expand Down Expand Up @@ -3097,7 +3098,7 @@ Slice::Gen::InterfaceVisitor::visitOperation(const OperationPtr& p)
string isConst = ((p->mode() == Operation::Nonmutating) || p->hasMetaData("cpp:const")) ? " const" : "";

string opName = amd ? (name + "Async") : fixKwd(name);
string deprecateSymbol = getDeprecateSymbol(p, interface);
string deprecateSymbol = getDeprecateSymbol(p);

H << sp;
if (comment)
Expand Down
68 changes: 34 additions & 34 deletions cpp/src/slice2cs/Gen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,28 +83,35 @@ namespace
return "???";
}

string getDeprecateReason(const ContainedPtr& p1, const ContainedPtr& p2, const string& type)
string getDeprecateReason(const ContainedPtr& p1, const string& type)
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if we should generate a special doc-comment message for deprecated operation and the like.

But we should at least give this helper function a clearer/consistent name, like getDocCommentDeprecationReason.

{
string deprecateMetadata, deprecateReason;
if (p1->findMetaData("deprecate", deprecateMetadata) ||
(p2 != 0 && p2->findMetaData("deprecate", deprecateMetadata)))
string deprecateReason;
if (p1->isDeprecated(true))
{
deprecateReason = "This " + type + " has been deprecated.";
const string prefix = "deprecate:";
if (deprecateMetadata.find(prefix) == 0 && deprecateMetadata.size() > prefix.size())
if (auto reason = p1->getDeprecateReason(true))
{
deprecateReason = deprecateMetadata.substr(prefix.size());
deprecateReason = *reason;
}
else
{
deprecateReason = "This " + type + " has been deprecated.";
}
}
return deprecateReason;
}

void emitDeprecate(const ContainedPtr& p1, const ContainedPtr& p2, Output& out, const string& type)
void emitObsoleteAttribute(const ContainedPtr& p1, Output& out)
{
string reason = getDeprecateReason(p1, p2, type);
if (!reason.empty())
if (p1->isDeprecated(true))
{
out << nl << "[global::System.Obsolete(\"" << reason << "\")]";
if (auto reason = p1->getDeprecateReason(true))
{
out << nl << "[global::System.Obsolete(\"" << *reason << "\")]";
}
else
{
out << nl << "[global::System.Obsolete]";
}
}
}

Expand Down Expand Up @@ -1678,8 +1685,7 @@ Slice::CsVisitor::writeDocCommentTaskAsyncAMI(
void
Slice::CsVisitor::writeDocCommentAMD(const OperationPtr& p, const string& extraParam)
{
InterfaceDefPtr interface = p->interface();
string deprecateReason = getDeprecateReason(p, interface, "operation");
string deprecateReason = getDeprecateReason(p, "operation");

StringList summaryLines;
StringList remarksLines;
Expand Down Expand Up @@ -2326,8 +2332,8 @@ Slice::Gen::TypesVisitor::visitExceptionStart(const ExceptionPtr& p)
ExceptionPtr base = p->base();

_out << sp;
writeDocComment(p, getDeprecateReason(p, 0, "type"));
emitDeprecate(p, 0, _out, "type");
writeDocComment(p, getDeprecateReason(p, "type"));
emitObsoleteAttribute(p, _out);
emitAttributes(p);
emitComVisibleAttribute();
//
Expand Down Expand Up @@ -2575,7 +2581,7 @@ Slice::Gen::TypesVisitor::visitStructStart(const StructPtr& p)
string ns = getNamespace(p);
_out << sp;

emitDeprecate(p, 0, _out, "type");
emitObsoleteAttribute(p, _out);

emitAttributes(p);
emitPartialTypeAttributes();
Expand Down Expand Up @@ -2830,8 +2836,8 @@ Slice::Gen::TypesVisitor::visitEnum(const EnumPtr& p)
const bool explicitValue = p->explicitValue();

_out << sp;
emitDeprecate(p, 0, _out, "type");
writeDocComment(p, getDeprecateReason(p, 0, "type"));
emitObsoleteAttribute(p, _out);
writeDocComment(p, getDeprecateReason(p, "type"));
emitAttributes(p);
emitGeneratedCodeAttribute();
_out << nl << "public enum " << name;
Expand Down Expand Up @@ -2951,7 +2957,7 @@ Slice::Gen::TypesVisitor::visitDataMember(const DataMemberPtr& p)

_out << sp;

emitDeprecate(p, cont, _out, "member");
emitObsoleteAttribute(p, _out);

string type = typeToString(p->type(), ns, isOptional);
string dataMemberName = fixId(p->name(), baseTypes, isClass);
Expand Down Expand Up @@ -3291,7 +3297,7 @@ Slice::Gen::ProxyVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)
InterfaceList bases = p->bases();

_out << sp;
writeDocComment(p, getDeprecateReason(p, 0, "interface"));
writeDocComment(p, getDeprecateReason(p, "interface"));
emitGeneratedCodeAttribute();
_out << nl << "public interface " << name << "Prx : ";

Expand Down Expand Up @@ -3334,7 +3340,7 @@ Slice::Gen::ProxyVisitor::visitOperation(const OperationPtr& p)
vector<string> inParams = getInParams(p, ns);
ParamDeclList inParamDecls = p->inParameters();
string retS = typeToString(p->returnType(), ns, p->returnIsOptional());
string deprecateReason = getDeprecateReason(p, interface, "operation");
string deprecateReason = getDeprecateReason(p, "operation");

{
//
Expand All @@ -3346,10 +3352,7 @@ Slice::Gen::ProxyVisitor::visitOperation(const OperationPtr& p)
p,
deprecateReason,
"<param name=\"" + context + "\">The Context map to send with the invocation.</param>");
if (!deprecateReason.empty())
{
_out << nl << "[global::System.Obsolete(\"" << deprecateReason << "\")]";
}
emitObsoleteAttribute(p, _out);
_out << nl << retS << " " << name << spar << getParams(p, ns)
<< (getUnqualified("Ice.OptionalContext", ns) + " " + context + " = new " +
getUnqualified("Ice.OptionalContext", ns) + "()")
Expand All @@ -3371,10 +3374,7 @@ Slice::Gen::ProxyVisitor::visitOperation(const OperationPtr& p)
"<param name=\"" + context + "\">Context map to send with the invocation.</param>",
"<param name=\"" + progress + "\">Sent progress provider.</param>",
"<param name=\"" + cancel + "\">A cancellation token that receives the cancellation requests.</param>");
if (!deprecateReason.empty())
{
_out << nl << "[global::System.Obsolete(\"" << deprecateReason << "\")]";
}
emitObsoleteAttribute(p, _out);
_out << nl << taskResultType(p, ns);
_out << " " << p->name() << "Async" << spar << inParams
<< (getUnqualified("Ice.OptionalContext", ns) + " " + context + " = new " +
Expand Down Expand Up @@ -3470,7 +3470,7 @@ Slice::Gen::OpsVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)
string opIntfName = "Operations";

_out << sp;
writeDocComment(p, getDeprecateReason(p, 0, "interface"));
writeDocComment(p, getDeprecateReason(p, "interface"));
emitGeneratedCodeAttribute();
_out << nl << "public interface " << name << opIntfName << '_';
if (bases.size() > 0)
Expand Down Expand Up @@ -3513,11 +3513,11 @@ Slice::Gen::OpsVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)
{
writeDocComment(
op,
getDeprecateReason(op, p, "operation"),
getDeprecateReason(op, "operation"),
"<param name=\"" + args.back() + "\">The Current object for the invocation.</param>");
}
emitAttributes(op);
emitDeprecate(op, op, _out, "operation");
emitObsoleteAttribute(op, _out);
emitGeneratedCodeAttribute();
_out << nl << retS << " " << opName << spar << params << epar << ";";
}
Expand Down Expand Up @@ -3587,7 +3587,7 @@ Slice::Gen::HelperVisitor::visitInterfaceDefStart(const InterfaceDefPtr& p)
vector<string> args = getArgs(op);
vector<string> argsAMI = getInArgs(op);

string deprecateReason = getDeprecateReason(op, p, "operation");
string deprecateReason = getDeprecateReason(op, "operation");

ParamDeclList inParams = op->inParameters();
ParamDeclList outParams = op->outParameters();
Expand Down
16 changes: 2 additions & 14 deletions cpp/src/slice2java/Gen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,13 +86,6 @@ namespace
return name;
}

bool isDeprecated(const ContainedPtr& p1, const ContainedPtr& p2)
{
string deprecateMetadata;
return p1->findMetaData("deprecate", deprecateMetadata) ||
(p2 != 0 && p2->findMetaData("deprecate", deprecateMetadata));
}

bool isValue(const TypePtr& type)
{
BuiltinPtr b = dynamic_pointer_cast<Builtin>(type);
Expand Down Expand Up @@ -1625,15 +1618,10 @@ Slice::JavaVisitor::writeDispatch(Output& out, const InterfaceDefPtr& p)

out << sp;
writeHiddenDocComment(out);
for (OperationList::iterator r = allOps.begin(); r != allOps.end(); ++r)
for (const OperationPtr& op : allOps)
Copy link
Member

Choose a reason for hiding this comment

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

I would add a TODO: temporary.

{
//
// Suppress deprecation warnings if this method dispatches to a deprecated operation.
//
OperationPtr op = *r;
InterfaceDefPtr interface = op->interface();
assert(interface);
if (isDeprecated(op, interface))
if (op->isDeprecated(true))
{
out << nl << "@SuppressWarnings(\"deprecation\")";
break;
Expand Down
Loading