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

Objective-C++ deprecation issues #166

Open
sirnacnud opened this issue Apr 19, 2024 · 6 comments
Open

Objective-C++ deprecation issues #166

sirnacnud opened this issue Apr 19, 2024 · 6 comments

Comments

@sirnacnud
Copy link

sirnacnud commented Apr 19, 2024

I am running in to an issue with the new deprecation support. I have an interface that is marked as deprecated, and I am generating Objective-C++ files, but looks like that generator wasn't changed in the deprecation support. When I try to compile the Objective-C++ files, it triggers a deprecation warning (or compile error if warnings are errors), as it is using the deprecated C++ type.

The problem is the ::MyInterface C++ type used in the class is deprecated. Does anyone have thoughts on how to resolve this?

idl:

# interface comment
#
# @deprecated Use someother interface
my_interface = interface +o {
  # @deprecated Use someother method
  method_a(value:i32);
}

my_interface.hpp:

// AUTOGENERATED FILE - DO NOT MODIFY!
// This file was generated by Djinni from test.djinni

#pragma once

#include <cstdint>

/**
 * interface comment
 *
 * @deprecated Use someother interface
 */
class [[deprecated("Use someother interface")]] MyInterface {
public:
    virtual ~MyInterface() {}

    /** @deprecated Use someother method */
    [[deprecated("Use someother method")]]
    virtual void method_a(int32_t value) = 0;
};

MyInterface+Private.h:

// AUTOGENERATED FILE - DO NOT MODIFY!
// This file was generated by Djinni from test.djinni

#include "my_interface.hpp"
#include <memory>

static_assert(__has_feature(objc_arc), "Djinni requires ARC to be enabled for this file");

@protocol MyInterface;

namespace djinni_generated {

class MyInterface
{
public:
    using CppType = std::shared_ptr<::MyInterface>;
    using CppOptType = std::shared_ptr<::MyInterface>;
    using ObjcType = id<MyInterface>;

    using Boxed = MyInterface;

    static CppType toCpp(ObjcType objc);
    static ObjcType fromCppOpt(const CppOptType& cpp);
    static ObjcType fromCpp(const CppType& cpp) { return fromCppOpt(cpp); }

private:
    class ObjcProxy;
};

}  // namespace djinni_generated
@a4z
Copy link
Contributor

a4z commented Apr 24, 2024

Hi, I am not sure if I understand the problem correctly.
Is it that the __deprecated_msg("...") in the Objective-C part is missing?

Mabye @eakoli can help more than I, since the deprecation feature comes from him.

@sirnacnud
Copy link
Author

The problematic code is this in the Objective-C++ generated conversion code. The ::MyInterface is deprecated in C++, so this will trigger a deprecation warning when compiling the above header MyInterface+Private.h file.

class MyInterface
{
public:
    // Theses are deprecated in C++
    using CppType = std::shared_ptr<::MyInterface>;
    using CppOptType = std::shared_ptr<::MyInterface>;
    ...
};

As a work around we can mark this C++ class as deprecated and that does eliminate the warning. I just wasn't for sure if this is the correct fix.

class [[deprecated("Use someother interface")]] MyInterface 
{
public:
    using CppType = std::shared_ptr<::MyInterface>;
    using CppOptType = std::shared_ptr<::MyInterface>;
    ...
};

@jothepro
Copy link
Contributor

In pydjinni I've worked around the issue by disabling deprecation warnings in the affected code, e.g. like this:

#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wdeprecated-declarations"
class MyInterface
{
public:
    // Theses are deprecated in C++
    using CppType = std::shared_ptr<::MyInterface>;
    using CppOptType = std::shared_ptr<::MyInterface>;
    ...
};
#pragma clang diagnostic pop

Not sure if that's the smartest approach, though.

If marking the generated translator class as deprecated does work as you describe, that sounds like a really nice solution to me!

@eakoli
Copy link
Contributor

eakoli commented Apr 25, 2024

Yea, unfortunately that is one of the issue with marking code as deprecated, as sometimes those methods / types still need to be used in the implementation. So you end up with code that will generate warnings.

The only real solution is to disable the warnings, which would need to be done in all the generated code and implementations of the exposed interfaces. This could be problematic if it's done automatically as its possible that it hides other uses of deprecated items, that you actually DO want to know about.

My specific reason for adding this in was more to communicate to our android/iOS developers what APIs that they should move off of, and we dont have deprecation as an error, only a warning.

@sirnacnud
Copy link
Author

For the project I am using Djinni with, we have warnings defined as errors, so the Djinni generated code that has warnings can't be compiled. I understand not all projects choose this option.

From an API standpoint, I would think compiling the generated API shouldn't create any warnings. Only when the client code tries to use the deprecated Djinni APIs should it create a warning. I think from a conceptual standpoint it makes sense to declare this C++ proxy as deprecated as well, as all the types it handles converting are already deprecated. This does solve the warnings as I mentioned above. I don't think there is any risk in hiding other unrelated deprecation warnings, as the proxy only has a few methods for converting from/to C++/Objective-C. Is there any objection to this? I can push up a PR for it.

@a4z
Copy link
Contributor

a4z commented May 4, 2024

Maybe @eakoli is the best person to answer that question since he introduced the feature.

For me, it's hard to estimate what this would break on existing code.
I kind of agree that compiling the generated API should probably not create warnings, and what Duncan suggests sounds reasonable, but personally, I have no use cases in my current projects affected by that, so I do not feel I am able to give a qualified statement on that topic.

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

No branches or pull requests

4 participants