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.

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 Swift dependency on non-public C++ APIs #2648

Closed
wants to merge 2 commits into from

Conversation

externl
Copy link
Member

@externl externl commented Aug 14, 2024

This PR removes Swift's dependency on non public C++ APIs.

Namely:

  • We don't call C++'s traceSlicing. This means we'll log every call now in Swift. Currently C++ caches ids and only logs once per id, which seems a bit excessive to me.
  • Remove calls to escapeString. We only use this to escape facets and category names when throwing a few exceptions. Not having this is not 100% correct. We should either copy the logic into Swift or make the C++ function public (or just not worry about it) in a future PR. I'm not sure how we handle this in Python.
  • Remove errorToString and errorToStringDNS which were just wrappers of their IceInternal counterpart. They were untested and unused.

@externl externl added the swift label Aug 14, 2024
@externl externl added this to the 3.8.0 milestone Aug 14, 2024
slicingCat: "Slicing",
logger: LoggerWrapper(handle: communicator.getLogger()))
let kind = sliceType == SliceType.ExceptionSlice ? "exception" : "object"
communicator.getLogger().trace(category: "Slicing", message: "unknown \(kind) type '\(typeId)'")
Copy link
Member

Choose a reason for hiding this comment

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

Should we apply this change to other mappings?

@bernardnormier
Copy link
Member

I don't understand the motivation for this PR. What's wrong with depending on symbols defined in IceImpl?

We have many ICE_API symbols in IceImpl (e.g. used by the generated code). And there many undocumented APIs in namespace Ice (say ConnectionI). For me, the distinction between namespace Ice and IceImpl is fairly meaningless.

Then:

We don't call C++'s traceSlicing. This means we'll log every call now in Swift. Currently C++ caches ids and only logs once per id, which seems a bit excessive to me.

This is a rather unfortunate change. In all langue mappings, the have this "duplicate log suppression" logic for class/exception slicing diagnostics. But now, you want to change this logic for Swift and embrace duplicate and very verbose tracing?

Remove calls to escapeString. We only use this to escape facets and category names when throwing a few exceptions

I would rather keep this escapeString as it's more correct and consistent with what we do in other language mappings.

@externl
Copy link
Member Author

externl commented Aug 14, 2024

I don't understand the motivation for this PR. What's wrong with depending on symbols defined in IceImpl?

These are not part of the public headers, so they're not part of the public API and will not be in the framework. I've updated the title and description.

We have many ICE_API symbols in IceImpl (e.g. used by the generated code). And there many undocumented APIs in namespace Ice (say ConnectionI). For me, the distinction between namespace Ice and IceImpl is fairly meaningless.

Then:

We don't call C++'s traceSlicing. This means we'll log every call now in Swift. Currently C++ caches ids and only logs once per id, which seems a bit excessive to me.

This is a rather unfortunate change. In all langue mappings, the have this "duplicate log suppression" logic for class/exception slicing diagnostics. But now, you want to change this logic for Swift and embrace duplicate and very verbose tracing?

I'm also fine applying the same logic in Swift or making the C++ API public.

Remove calls to escapeString. We only use this to escape facets and category names when throwing a few exceptions

I would rather keep this escapeString as it's more correct and consistent with what we do in other language mappings.

I agree, we'll need to either make the API public or copy the logic into Swift.

@externl externl changed the title Remove Swift dependency on C++ IceInternal Remove Swift dependency on non-public C++ APIs Aug 14, 2024
@bernardnormier
Copy link
Member

For APIs that we use (traceSlicing, espaceString) from Swift (and presumably other C++-based language mappings), I would move them to "public" headers in include/Ice.

Even though they are in public headers, they remain undocumented and in namespace IceImpl.

@externl
Copy link
Member Author

externl commented Aug 14, 2024

Swift is actually the only language to use them.

It's not so obvious we can move these headers to be public. TraceUitl.h includes InstanceF.h. It might be cleaner to just copy this logic to Swift for this one function we use.

I think escapeString might already be in a public header. Need to double check.

The issue with escapeString is that we need to get the ToStringMode from communicator.

@bernardnormier
Copy link
Member

In theory, we should have the same "trace slicing" logic in MATLAB, but the code looks buggy.

 matlab % rg traceSkipSlice
lib/+IceInternal/EncapsDecoder10.m
185:            %obj.is.traceSkipSlice(obj.typeId, obj.sliceType);

InputStream does not have a traceSkipSlice in MATLAB.

@externl
Copy link
Member Author

externl commented Aug 14, 2024

Looking a little more it's actually a bit worse. I think I can fix escapeString to not use instance and just get the toStringMode from properties.

However we're currently using Instance in Swift's Communicator to get the defaultAndOverrides for a few things. We use these to create our own Swift version of DefaultsAndOverrides that we use in OutputStream and InputStream.

@externl externl marked this pull request as draft August 14, 2024 18:16
@externl externl closed this Aug 16, 2024
@externl
Copy link
Member Author

externl commented Aug 16, 2024

Closing this for now as there's no simple way to not rely all of the internal headers. It's fine that they're not in the XCFramework as we always do a build from the repo were the private headers are available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants