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

atdcpp: new backend #404

Merged
merged 47 commits into from
May 6, 2024
Merged

atdcpp: new backend #404

merged 47 commits into from
May 6, 2024

Conversation

elrandar
Copy link
Contributor

@elrandar elrandar commented Mar 4, 2024

New atd backend for C++

  • Depends on rapidjson library.
  • Implemented ✅
    • Structs
    • Tuples
    • Typedefs
    • SumTypes (Variants)
    • Dict and object representations for assoc list
    • Untyped things through string representation
    • Wrapping
    item: string wrap <cpp t="int" wrap="std::stoi" unwrap="std::to_string">;
    • Definition of cpp includes with the following syntax
    <cpp include="<stdint.h>">
    • Generated files split between output_atd.hpp and output_atd.cpp for better readability
    • Recursion support thanks to C++ forward declarations (able to generate a type that contains itself when its not directly)
    • Direct recursion support when wrapping with a pointer with templatize annotation (wraps into a type templated by the previous type).
    type recursive_record2 = {
      id: int;
      flag: bool;
      children: recursive_record2 nullable wrap <cpp templatize t="std::shared_ptr" wrap="_atd_wrap_shared_ptr" unwrap="_atd_unwrap_shared_ptr">;}
    • Possibility to specify namespace <cpp namespace="my_namespace">, will default to atd::
    • Support for enum repr for variants. Will still default to std::variant. Only works for sumtypes without fields.
    type enum_sumtype = [
     | A
     | B
     | C
    ] <cpp repr="enum">
  • ToDo
    • Solve issue where if you define multiple types with generics, that depend on each other, the order of c++ declarations will not necessarily be correct, and will prevent compilation unless header is manually edited
      • See comment below

@elrandar
Copy link
Contributor Author

elrandar commented Apr 5, 2024

issue where if you define multiple types with generics, that depend on each other, the order of c++ declarations will not necessarily be correct, and will prevent compilation unless header is manually edited

This issue is related to the way of how atd will sort type declarations. This problem arises because there is a circular dependancy, but one order still works, since C++ only need to know the memory taken by the type (full definition) if it occurs in a struct, without being wrapped in a pointer or a list.

Exemple of that issue:
test.atd

type field =
  {
    field : string;
  }

type 'a combinator =
  {
    integer : int;
    list_of_things : 'a list;
  }

type 'a query' =
  [ Elt of 'a
  | Or of 'a query' combinator
  ]

type fq = field query'

test.hpp

namespace FieldQuery {
    namespace Types {
        // Original type: _field_query = [ ... | Elt of ... | ... ]
        struct Elt
        {
            typedefs::Field value;
            static void to_json(const Elt &e, rapidjson::Writer<rapidjson::StringBuffer> &writer);
        };
        // Original type: _field_query = [ ... | Or of ... | ... ]
        struct Or
        {
            typedefs::FieldQueryCombinator value;
            static void to_json(const Or &e, rapidjson::Writer<rapidjson::StringBuffer> &writer);
        };
    }

    typedefs::FieldQuery from_json(const rapidjson::Value &x);
    // ...
}

struct FieldQueryCombinator {
    int integer;
    std::vector<typedefs::FieldQuery> list_of_things;

    static FieldQueryCombinator from_json(const rapidjson::Value & doc);
    // ...
};

As you can see, defining FieldQueryCombinator before FieldQuery would solve the issue.
In the cases where that is happening, I fixed by just wrapping with a pointer in c++, because otherwise I think it would require too many changes on the frontend.
Maybe I should open an issue for that, but not sure if it applies to any other languages.

@elrandar elrandar marked this pull request as ready for review April 8, 2024 04:03
@elrandar
Copy link
Contributor Author

elrandar commented Apr 8, 2024

I think this PR is now ready for review.
I'm quite happy with the code produced, especially compared to the dlang backend. The separation between interface and implementation really helps when actually using the types.
The main problem that still needs to be addressed is how to cleanly integrate rapidjson to the project, since it is needed to run and compile c++ code.

I think the best way would be to add the install step to the CI, but I'm not sure about it.
If you could tell me which way is preferred @mjambon @Khady.

@mjambon
Copy link
Collaborator

mjambon commented Apr 9, 2024

The main problem that still needs to be addressed is how to cleanly integrate rapidjson to the project, since it is needed to run and compile c++ code.

I assume we don't need rapidjson to compile the OCaml program atdcpp. Rapidjson is a runtime library needed for testing atdcpp, so I suggest installing the dependency from a reliable source from the script ~/atd/.circleci/setup-system. This script will also be used by other developers of the ATD project to set things up without being familiar with all the target ecosystems.

Copy link
Collaborator

@mjambon mjambon left a comment

Choose a reason for hiding this comment

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

At the glance, the code generator and the generated code look fine to me. As long as there's good test coverage, it's great. I'm no C++ expert so if you're worried about some choices you made for the generated C++ code, you should find someone else.

atdcpp/src/lib/Codegen.mli Show resolved Hide resolved
atdcpp/src/lib/Codegen.mli Show resolved Hide resolved
atdcpp/test/cpp-tests/dune Outdated Show resolved Hide resolved
@mjambon
Copy link
Collaborator

mjambon commented Apr 9, 2024

I didn't say it but having C++ support is a massive achievement. Thank you! You should make a big announcement once it's in a usable state (I suggest as soon as it's available in opam or earlier).

@elrandar
Copy link
Contributor Author

elrandar commented May 6, 2024

I made the necessary changes, I think it should be mergeable as is.

@mjambon mjambon merged commit 8aab3c9 into master May 6, 2024
2 checks passed
@Khady Khady deleted the alexandre/atdcpp branch May 7, 2024 06:19
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

Successfully merging this pull request may close these issues.

2 participants