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

Remove protected metadata #3382

Merged
merged 10 commits into from
Jan 21, 2025
Merged

Remove protected metadata #3382

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
63d8efd
Remove protected metadata
pepone Jan 20, 2025
0559e44
Update cpp/test/Slice/errorDetection/WarningInvalidMetadata.ice
pepone Jan 20, 2025
e9a0598
Update cpp/test/Slice/errorDetection/WarningInvalidMetadata.ice
pepone Jan 20, 2025
e68ffaf
Update cpp/test/Slice/errorDetection/WarningInvalidMetadata.ice
pepone Jan 20, 2025
bde98ce
Cleanup tests
pepone Jan 20, 2025
c04f91d
C# test fixes
pepone Jan 21, 2025
cda0e8f
Merge remote-tracking branch 'origin/main' into issue-3360
pepone Jan 21, 2025
db95aa7
Missing separator
pepone Jan 21, 2025
9b1fa66
Fix build failure
pepone Jan 21, 2025
496397d
format fixes
pepone Jan 21, 2025
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
7 changes: 0 additions & 7 deletions cpp/src/Slice/MetadataValidation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,13 +113,6 @@ Slice::validateMetadata(const UnitPtr& p, string_view prefix, map<string, Metada
};
knownMetadata.emplace("marshaled-result", std::move(marshaledResultInfo));

// "protected"
MetadataInfo protectedInfo = {
.validOn = {typeid(ClassDecl), typeid(Slice::Exception), typeid(Struct), typeid(DataMember)},
.acceptedArgumentKind = MetadataArgumentKind::NoArguments,
};
knownMetadata.emplace("protected", std::move(protectedInfo));

// "suppress-warning"
MetadataInfo suppressWarningInfo = {
.validOn = {typeid(Unit)},
Expand Down
46 changes: 3 additions & 43 deletions cpp/src/slice2cpp/Gen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2258,55 +2258,15 @@ Slice::Gen::DataDefVisitor::visitClassDefEnd(const ClassDefPtr& p)

// Emit data members. Access visibility may be specified by metadata.
const DataMemberList dataMembers = p->dataMembers();
const bool prot = p->hasMetadata("protected");
bool inProtected = false;
bool needSp = true;
Copy link
Member

Choose a reason for hiding this comment

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

What does the formatting look like without needSp?
It seems like this field was for emitting a newline before the first data member, more than it was for protected stuff.


for (const auto& dataMember : dataMembers)
{
if (prot || dataMember->hasMetadata("protected"))
{
if (!inProtected)
{
H.dec();
H << sp << nl << "protected:" << sp;
H.inc();
inProtected = true;
needSp = false;
}
}
else
{
if (inProtected)
{
H.dec();
H << sp << nl << "public:" << sp;
H.inc();
inProtected = false;
needSp = false;
}
}

if (needSp)
{
H << sp;
needSp = false;
}

emitDataMember(dataMember);
}

if (inProtected)
{
H << sp;
}
else
{
H.dec();
H << sp << nl << "protected:";
H.inc();
inProtected = true;
}
H.dec();
H << sp << nl << "protected:";
H.inc();

H << nl << name << "(const " << name << "&) = default;";
H << sp << nl << _dllMemberExport << "[[nodiscard]] ::Ice::ValuePtr _iceCloneImpl() const override;";
Expand Down
12 changes: 1 addition & 11 deletions cpp/src/slice2cs/Gen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1582,7 +1582,6 @@ Slice::Gen::TypesVisitor::visitDataMember(const DataMemberPtr& p)
{
unsigned int baseTypes = 0;
bool isClass = false;
bool isProtected = false;
const bool isOptional = p->optional();

ContainedPtr cont = dynamic_pointer_cast<Contained>(p->container());
Expand All @@ -1609,7 +1608,6 @@ Slice::Gen::TypesVisitor::visitDataMember(const DataMemberPtr& p)
assert(cl);
baseTypes = DotNet::ICloneable;
isClass = true;
isProtected = cont->hasMetadata("protected") || p->hasMetadata("protected");
}

_out << sp;
Expand All @@ -1620,15 +1618,7 @@ Slice::Gen::TypesVisitor::visitDataMember(const DataMemberPtr& p)
string dataMemberName = fixId(p->name(), baseTypes, isClass);

emitAttributes(p);
if (isProtected)
{
_out << nl << "protected";
}
else
{
_out << nl << "public";
}
_out << ' ' << type << ' ' << dataMemberName;
_out << nl << "public" << ' ' << type << ' ' << dataMemberName;

bool addSemicolon = true;
if (isProperty)
Expand Down
8 changes: 1 addition & 7 deletions cpp/src/slice2java/Gen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3200,13 +3200,7 @@ Slice::Gen::TypesVisitor::visitDataMember(const DataMemberPtr& p)
out << nl << "@Deprecated";
}

// Access visibility for class data members can be controlled by metadata.
// If none is specified, the default is public.
if (cls && (p->hasMetadata("protected") || contained->hasMetadata("protected")))
{
out << nl << "protected " << s << ' ' << name << ';';
}
else if (optional)
if (optional)
{
out << nl << "private " << s << ' ' << name << ';';
}
Expand Down
71 changes: 9 additions & 62 deletions cpp/src/slice2matlab/Main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1386,72 +1386,19 @@ CodeVisitor::visitClassDefStart(const ClassDefPtr& p)
const DataMemberList members = p->dataMembers();
if (!members.empty())
{
if (p->hasMetadata("protected"))
out << nl << "properties";
out.inc();
for (const auto& q : members)
{
//
// All members are protected.
//
out << nl << "properties(Access=protected)";
out.inc();
for (const auto& member : members)
writeMemberDoc(out, q);
out << nl << fixIdent(q->name());
if (declarePropertyType(q->type(), q->optional()))
{
writeMemberDoc(out, member);
out << nl << fixIdent(member->name());
if (declarePropertyType(member->type(), member->optional()))
{
out << " " << typeToString(member->type());
}
}
out.dec();
out << nl << "end";
}
else
{
DataMemberList prot, pub;
for (const auto& member : members)
{
if (member->hasMetadata("protected"))
{
prot.push_back(member);
}
else
{
pub.push_back(member);
}
}
if (!pub.empty())
{
out << nl << "properties";
out.inc();
for (const auto& q : pub)
{
writeMemberDoc(out, q);
out << nl << fixIdent(q->name());
if (declarePropertyType(q->type(), q->optional()))
{
out << " " << typeToString(q->type());
}
}
out.dec();
out << nl << "end";
}
if (!prot.empty())
{
out << nl << "properties(Access=protected)";
out.inc();
for (const auto& q : prot)
{
writeMemberDoc(out, q);
out << nl << fixIdent(q->name());
if (declarePropertyType(q->type(), q->optional()))
{
out << " " << typeToString(q->type());
}
}
out.dec();
out << nl << "end";
out << " " << typeToString(q->type());
}
}
out.dec();
out << nl << "end";
}

MemberInfoList allMembers;
Expand Down
12 changes: 1 addition & 11 deletions cpp/src/slice2php/Main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,19 +246,9 @@ CodeVisitor::visitClassDefStart(const ClassDefPtr& p)
if (!members.empty())
{
_out << sp;
bool isProtected = p->hasMetadata("protected");
for (const auto& member : members)
{
_out << nl;
if (isProtected || member->hasMetadata("protected"))
{
_out << "protected ";
}
else
{
_out << "public ";
}
_out << "$" << fixIdent(member->name()) << ";";
_out << nl << "public " << "$" << fixIdent(member->name()) << ";";
}
}

Expand Down
15 changes: 2 additions & 13 deletions cpp/src/slice2py/PythonUtil.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -672,18 +672,14 @@ Slice::Python::CodeVisitor::visitClassDefStart(const ClassDefPtr& p)
_out.inc();
_out << nl;
}
bool isProtected = p->hasMetadata("protected");

for (auto r = members.begin(); r != members.end(); ++r)
{
if (r != members.begin())
{
_out << ',' << nl;
}
_out << "('";
if (isProtected || (*r)->hasMetadata("protected"))
{
_out << '_';
}
_out << fixIdent((*r)->name()) << "', ";
writeMetadata((*r)->getMetadata());
_out << ", ";
Expand Down Expand Up @@ -1975,14 +1971,7 @@ Slice::Python::CodeVisitor::collectClassMembers(const ClassDefPtr& p, MemberInfo
for (const auto& member : p->dataMembers())
{
MemberInfo m;
if (p->hasMetadata("protected") || member->hasMetadata("protected"))
{
m.fixedName = "_" + fixIdent(member->name());
}
else
{
m.fixedName = fixIdent(member->name());
}
m.fixedName = fixIdent(member->name());
m.inherited = inherited;
m.dataMember = member;
allMembers.push_back(m);
Expand Down
24 changes: 0 additions & 24 deletions cpp/src/slice2rb/RubyUtil.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,9 +332,6 @@ Slice::Ruby::CodeVisitor::visitClassDefStart(const ClassDefPtr& p)
//
if (!members.empty())
{
bool prot = p->hasMetadata("protected");
DataMemberList protectedMembers;

_out << sp << nl << "attr_accessor ";
for (auto q = members.begin(); q != members.end(); ++q)
{
Expand All @@ -343,27 +340,6 @@ Slice::Ruby::CodeVisitor::visitClassDefStart(const ClassDefPtr& p)
_out << ", ";
}
_out << ":" << fixIdent((*q)->name(), IdentNormal);
if (prot || (*q)->hasMetadata("protected"))
{
protectedMembers.push_back(*q);
}
}

if (!protectedMembers.empty())
{
_out << nl << "protected ";
for (auto q = protectedMembers.begin(); q != protectedMembers.end(); ++q)
{
if (q != protectedMembers.begin())
{
_out << ", ";
}
//
// We need to list the symbols of the reader and the writer (e.g., ":member" and ":member=").
//
_out << ":" << fixIdent((*q)->name(), IdentNormal) << ", :" << fixIdent((*q)->name(), IdentNormal)
<< '=';
}
}
}

Expand Down
4 changes: 2 additions & 2 deletions cpp/test/Ice/objects/Test.ice
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,15 +61,15 @@ class D
bool postUnmarshalInvoked;
}

["protected"] class E
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 remove these tests since they were apparently all about testing protected.

class E
{
int i;
string s;
}

class F
{
["protected"] E e1;
E e1;
E e2;
}

Expand Down
2 changes: 1 addition & 1 deletion cpp/test/Ice/optional/Test.ice
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ exception RequiredException extends OptionalException
class OptionalWithCustom
{
optional(1) SmallStructList l;
["protected"] optional(2) SmallStructList lp;
Copy link
Member

Choose a reason for hiding this comment

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

Same here. lp is the same as l (great name) except protected.

optional(2) SmallStructList lp;
}

class E
Expand Down
2 changes: 1 addition & 1 deletion cpp/test/Ice/optional/TestAMD.ice
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ exception RequiredException extends OptionalException
class OptionalWithCustom
{
optional(1) SmallStructList l;
["protected"] optional(2) SmallStructList lp;
optional(2) SmallStructList lp;
}

class E
Expand Down
1 change: 0 additions & 1 deletion cpp/test/Slice/errorDetection/WarningInvalidMetadata.err
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ WarningInvalidMetadata.ice:44: warning: ignoring duplicate metadata: 'deprecated
WarningInvalidMetadata.ice:48: warning: ignoring unknown metadata: 'unknown'
WarningInvalidMetadata.ice:48: warning: ignoring unknown metadata: 'cpp:unknown'
WarningInvalidMetadata.ice:48: warning: ignoring unknown metadata: 'bad:unknown'
WarningInvalidMetadata.ice:56: warning: the 'protected' metadata does not take any arguments
WarningInvalidMetadata.ice:56: warning: the 'cpp:array' metadata does not take any arguments
WarningInvalidMetadata.ice:56: warning: 'cpp:array' metadata can only be applied to operation parameters and return types
WarningInvalidMetadata.ice:62: warning: 'cpp:type' metadata cannot be applied to operations with void return type
Expand Down
8 changes: 4 additions & 4 deletions cpp/test/Slice/errorDetection/WarningInvalidMetadata.ice
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ class Container
int i;

// Passing an argument to metadata that doesn't take arguments is disallowed.
["protected:foo", "cpp:array:foo"]
["cpp:array:foo"]
Blob blob;
}

Expand Down Expand Up @@ -100,15 +100,15 @@ class P
// Metadata is shared between forward declarations and definitions.
// We allow duplicate metadata, but require that that metadata must be identical.

["java:nonsense", "deprecated:hello", "protected"]
["java:nonsense", "deprecated:hello"]
class K;
pepone marked this conversation as resolved.
Show resolved Hide resolved

["java:nonsense", "deprecated:goodbye", "protected"]
["java:nonsense", "deprecated:goodbye"]
class K
pepone marked this conversation as resolved.
Show resolved Hide resolved
{
}

["java:nonsense", "deprecated", "protected"]
["java:nonsense", "deprecated"]
class K;
pepone marked this conversation as resolved.
Show resolved Hide resolved

}
16 changes: 0 additions & 16 deletions csharp/test/Ice/objects/AllTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,22 +178,6 @@ public static InitialPrx allTests(global::Test.TestHelper helper)

output.WriteLine("ok");

output.Write("testing protected members... ");
output.Flush();
var e = (EI)initial.getE();
test(e != null && e.checkValues());
System.Reflection.BindingFlags flags = System.Reflection.BindingFlags.NonPublic |
System.Reflection.BindingFlags.Public |
System.Reflection.BindingFlags.Instance;
test(!typeof(E).GetField("i", flags).IsPublic && !typeof(E).GetField("i", flags).IsPrivate);
test(!typeof(E).GetField("s", flags).IsPublic && !typeof(E).GetField("s", flags).IsPrivate);
var f = (FI)initial.getF();
test(f.checkValues());
test(((EI)f.e2).checkValues());
test(!typeof(F).GetField("e1", flags).IsPublic && !typeof(F).GetField("e1", flags).IsPrivate);
test(typeof(F).GetField("e2", flags).IsPublic && !typeof(F).GetField("e2", flags).IsPrivate);
output.WriteLine("ok");

output.Write("getting K... ");
{
output.Flush();
Expand Down
Loading
Loading