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

Generate aliases that resolve to the latest minor version of the data type #193

Open
Cherish-forever opened this issue Mar 1, 2021 · 11 comments
Labels
feature New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@Cherish-forever
Copy link

I think nunavut generate source file without version, so that user application will don`t care about protocol version.
user application include by different protocol version header file to change protocol version.
application code will use the variable or struct or api without version.

@pavel-kirienko
Copy link
Member

My own experience also indicates that it would be valuable to be able to omit the minor version number in some cases. Dropping the major version may cause nasty consequences since there is no wire compatibility guarantee across different major versions, but the minor one is usually safe to omit making the implementation default to the latest one. This idea does not need to be limited to C only, of course.

However, if we did it the way you described, C/C++ applications that leverage different minor versions of a data type would violate the ODR since there would be different definitions under the same name available in different files. A robust solution is to make Nunavut emit an alias for every major version of every data type that simply resolves to the latest minor version; such alias necessarily has to be placed in a separate file.

For example, if you have Type.1.1, Type.1.2, and Type.2.0, Nunavut would produce the following outputs (assume C in this case but it holds for other targets):

  • Type_1_1.h --- implements Type.1.1
  • Type_1_2.h --- implements Type.1.2
  • Type_2_0.h --- implements Type.2.0
  • Type_1.h --- defines alias for Type_1_2 and includes Type_1_2.h.

@thirtytwobits do you support this? @Cherish-forever would you like to work on this?

@pavel-kirienko pavel-kirienko changed the title Some idea for generate source files Generate aliases that resolve to the latest minor version of the data type Mar 1, 2021
@pavel-kirienko pavel-kirienko added the feature New feature or request label Mar 1, 2021
@Cherish-forever
Copy link
Author

@pavel-kirienko
Keep major is better.

@pavel-kirienko
Copy link
Member

@thirtytwobits if you greenlighted this we could ask @Cherish-forever to help us implement it.

@thirtytwobits
Copy link
Member

@Cherish-forever and @pavel-kirienko , yeah. I think this makes a lot of sense.

@thirtytwobits thirtytwobits added good first issue Good for newcomers help wanted Extra attention is needed labels Mar 17, 2021
@pavel-kirienko
Copy link
Member

So @Cherish-forever would you like to work on this?

@silverv
Copy link
Contributor

silverv commented Dec 31, 2021

Just to clarify, work for this is done in Python but needs to be implemented here for C and C++.

@pavel-kirienko
Copy link
Member

@thirtytwobits How would you go about implementing this for C++? We need to extend the list of data types for which we generate outputs with aliases. Given a list of DSDL types that contains ns.Type.1.0, we will need to generate not only ns/Type_1_0.hpp but also ns/Type_1.hpp. This differs from the implementation for Python because in Python you can define the aliases inside __init__.py by providing a custom Namespace.j2, which is a lot easier. Does it look to you that implementing this for C++ requires modifications to the Nunavut core?

This issue is kinda important right now because the high-level interfaces in libcyphal are going to depend on DSDL-generated types, and we don't want them to depend on a specific minor version (at least not always).

@thirtytwobits
Copy link
Member

We could overload the namespace concept such that, when enabled in C++ generation, we generate ns/Type_{major}.h that is simply using directives:

// Example of Type_1.h

#include "ns/Type_1_1.h"

namespace ns
{
using Type_1 = ns::Type_1_1;
}

Is this what we're looking for here?

@pavel-kirienko
Copy link
Member

Yes.

@thirtytwobits
Copy link
Member

(for C++ and C this should be the same. I'll discuss c++ here)

In src/nunavut/lang/properties.yaml under the cpp configuration you'll see two keys:

    namespace_file_stem: _namespace_
    has_standard_namespace_files: false

Setting has_standard_namespace_files to true will cause the generator to fail because there is no C++ template for this. If you touch src/nunavut/lang/cpp/templates/Namespace.j2 and try again it will work by emitting _namespace_.hpp files for each folder it creates. You could then develop this template (and rename the stem... let's just call it MajorTypes) to have aliases for all major types in a given namespace:

    namespace_file_stem: MajorType
    has_standard_namespace_files: true
// example of uavcan/diagnostic/MajorTypes.hpp

#include "uavcan/diagnostic/Record_1_1.hpp"
#include "uavcan/diagnostic/Severity_1_0.hpp"

namespace uavcan
{
namespace diagnostic
{
using Record_1 = Record_1_1;
using Severity_1 = Severity_1_0;
}
}

// example use:
#include "uavcan/diagnostic/MajorTypes.hpp"

...
auto latest_record = new uavcan::diagnostic::Record_1();

This is a bit ugly, I admit. The alternative would require changes to the core to enable a "generate latest minor of major only" where the generator itself would only emit (for our example) uavcan/diagnostic/Record_1.hpp and uavcan/diagnostic/Severity_1.hpp under the uavcan/diagnostic namespace. The types in here would be named by their major and minor version but would come with the alias suggested by the header name.

Thoughts?

@pavel-kirienko
Copy link
Member

This is a bit ugly, I admit. The alternative would require changes to the core to enable a "generate latest minor of major only"

I think this is superior to the namespace header (MajorTypes.hpp) approach because, for large namespaces, dependency of the namespace header on all types in its namespace may be a problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants