Support for c-style arrays.

[hazby2002]

Calculate correct num_fields when there are nested c-style arrays in an aggregate. The idea is from #39 . The example in #39 will not trigger compile errors now, but the output is still not same as expected.

Whole support for c-style arrays need further works.

[ChemiCalChems]

It's cool that the example no longer causes compile errors, but until there is a complete solution that outputs the expected result, I'd rather have using the library in this way cause a compile error over it failing silently and providing the wrong output.

Merging in this pull request at this point would thus allow users to blindly trust the library to work in cases like these when it obviously shouldn't be trusted. I think this should be treated as a draft PR.

Maybe we should note the library's limitation in the README for the time being until we get this working completely.

[hazby2002]

@ChemiCalChems I agree. I will look into the code and find further solutions to the problem.

[liuzicheng1987]

@hazby2002 if there are any questions, don’t hesitate to ask. I can help you wherever help is needed.

You are certainly on the right track with your implementation of the Parser for c arrays, though.

[hazby2002]

Hi all, I am glad to say that the example in #39 has printed expected results. I have added some json tests for write operations. Read operations need further work.

[hazby2002]

Supporting read operations is not a simple task, and at the very least we should support compilation-time expression of nested braces so that they can be used for aggregates containing c-arrays. Maybe we should do this in PR-s afterward.

[hazby2002]

Hello everyone, once again I am glad to announce that simple c array support has been implemented. More tests are required on more complex cases (such as combination of c-arrays with Flatten, Rename, etc.)

[hazby2002]

My implementation uses reinterpret cast, which is dangerous. I am not sure whether there is any hidden danger.

[liuzicheng1987]

I think reinterpret cast is fine in principle...depends on how it is used, of course. I will take a look.

[liuzicheng1987]

@hazby2002 , this is great work. Were you really born in 2002? Very impressive.

I have noticed a couple of minor things:

• It appears that the read operation of your Parser_c_array.hpp simply repeats the code from Parser_array.hpp. There certainly is a fair degree of code duplication there. I think you could greatly simplify the code by having Parser_c_array::read simple call Parser_array::read.
• Your test seems to be testing three things at once. It is usually good practice to have one thing to be tested per test. Could you break that up into three tests?
• You have inherited from rfl::Result and rfl::Field and specialized that for your class. This violates quite a few C++ best practices. I think a better solution would to be to handle this special case inside the classes themselves. Even though I am not 100% sure what a good solution would like here.

But the important thing is that we now have a proof-of-concept that it can be done. Which is very cool. The rest can be fixed.

[hazby2002]

Yes, I am a fourth year undergraduate student.

I was exploring many ways and thus some codes do not follow best practices.

It appears that the read operation of your Parser_c_array.hpp simply repeats the code from Parser_array.hpp. There certainly is a fair degree of code duplication there. I think you could greatly simplify the code by having Parser_c_array::read simple call Parser_array::read.

OK. I simply copy the codes from Parser_array.hpp. The reason I didn't just call Parser_array::read is that at first I didn't know what template arguments R,W are and I'm afraid R/W differ between the two classes, but actually they seem to be same.

Your test seems to be testing three things at once. It is usually good practice to have one thing to be tested per test. Could you break that up into three tests?

OK. I was using Google Test and I tend to collect related tests into a single file.

You have inherited from rfl::Result and rfl::Field and specialized that for your class. This violates quite a few C++ best practices. I think a better solution would to be to handle this special case inside the classes themselves. Even though I am not 100% sure what a good solution would like here.

I tried to handle this special case inside the classes but the solution looks some complicated. I know what to do now.

[liuzicheng1987]

@hazby2002 , what is the current stage of this PR? Do you want me to take another look?

[hazby2002]

@liuzicheng1987 The PR is ready. I did git rebase and fixed the things in your comment. You can take a look at the last 4 commits.

[liuzicheng1987]

OK, I have taken a closer look at this. (Sorry it took me a while, but I was on a transcontinental flight. I was able to write code, but didn't have access to the internet.).

The first problem that I find is that the logic breaks apart as soon as you add more than one array:

struct Test1 {
  int ints1[3];
  int ints2[3];
};

void test() {
  Test1 test1 = {.ints1 = {1, 2, 3}, .ints2 = {4, 5, 6}};
  write_and_read(test1, R"({"ints1":[1,2,3],"ints2":[4,5,6]})");
}

This fails to compile with the following message:

/home/ubuntu/PRs/hazby2002/reflect-cpp/tests/json/test_c_array_class1.cpp:22:17: required from here /home/ubuntu/PRs/hazby2002/reflect-cpp/include/rfl/Result.hpp:237:53: error: could not convert ‘rfl::parsing::NamedTupleParser<rfl::json::Reader, rfl::json::Writer, false, rfl::Field<rfl::internal::StringLiteral<6>{std::array<char, 5>{"ints"}}, int [3]>, rfl::Field<rfl::internal::StringLiteral<6>{std::array<char, 5>{"ints"}}, int [3]> >::collect_errors<1>(const rfl::json::Reader&, const std::array<std::optional, 2>&, std::vector)::<lambda(rfl::Error&&)>(( & std::forward(( & _value_or_err))))’ from ‘Result<int [3]>’ to ‘Result<std::array<int, 3>>’ 237 | return _f(std::forward(_value_or_err));

For some reason, your logic for transform c arrays to std::arrays fails here.

I am generally a bit unhappy about the fact that we have to implement this workaround in rfl::Result - it seems like a violation of separation of concerns (I don't mind it nearly as much in rfl::Field, because that class is supposed to be concerned with these kind of problems, so it doesn't violate separation of concerns). I don't really have a good solution, though.

Finally, I do not think we need the forward declaration in here:

https://github.com/hazby2002/reflect-cpp/blob/main/include/rfl/internal/bind_to_tuple.hpp

Unless I am missing something, you could simply place bind_to_tuple at the bottom of the code and that would be fine.

[liuzicheng1987]

By the way...once again, this is very creative and very impressive.

[liuzicheng1987]

@hazby2002 , the problems seems to arise due the fact that we forced to return std::array in the Parser implementation since normal arrays cannot be returned.

To be honest, I am a bit unsure how to proceed here.

I think you have come up with a very impressive and creative solution, but supporting C arrays is adding a lot of complexity (also a lot of compile time complexity) for fairly limited value, since there is a perfectly fine alternative, std::array, which we already support.

I think we would have to find something that is more elegant otherwise we will always run into issues like this.

[hazby2002]

@liuzicheng1987 I agree. The key point is that we must have Result<int[3]> when c arrays cannot be returned. My initial solution is simply treat Result<int[3]> and Result<array<int, 3>> as the same. I don't have better solutions if we do not modify Result.

[liuzicheng1987]

Yes, obviously, it would be possible to overload the various constructors to support this. But it would add a lot of complexity and that is not really what rfl::Result is meant to be used for. At the moment, I don’t have a very elegant solution either, though. I will have to think about it.

[liuzicheng1987]

@hazby2002 Here is an idea that might work:

We introduce a special wrapper type, rfl::internal::Array<…>. When transforming structs from and to named tuples, we could wrap and unwrap the wrapper type. So then the field type would not be rfl::Field<„Name“, int[3]>, but rfl::Field<„Name“, internal::Array<int[3]>>. So the special case would have to be handled in from_named_tuple and to_named_tuple. And then everything else, Field, Result, Parser could work exactly like it always does, since our wrapper type could be moved and returned.

Do you get what I mean?

[hazby2002]

@liuzicheng1987 I get it. I will try to implement it in a few days.

[liuzicheng1987]

@hazby2002 , quick addendum: You would also have to adapt rfl/named_tuple_t. This works using ptr_named_tuple (and should continue to work that way, because otherwise we would not be able support non-copyable fields).

ptr_named_tuple should continue to simply return pointers to the original fields. But I think you can just adapt line 20 in this file:

https://github.com/hazby2002/reflect-cpp/blob/main/include/rfl/named_tuple_t.hpp

You could replace std::remove_cvref_t<std::remove_pointer_t<T>> with something like wrap_in_rfl_array_t<std::remove_cvref_t<std::remove_pointer_t<T>>> and that should do the trick.

If you need any help, let me know. In the meantime, I will also think about ways we could reduce the compile time complexity of identifying the arrays.

[hazby2002]

@liuzicheng1987 I was stuck when trying to remove the specialization of Result<c-array>. IsReader and IsWriter will cause the instantiation of Result<c-array>, and a compile error where value() tries to return a c-array. In a word, we must modify everything that might cause the instantiation of Result<c-array>. By the way, I have implemented your idea and tests are passed (only when I let AreReaderAndWriter=true and let json::read return auto).

[liuzicheng1987]

@hazby2002 Yes...I see the problem.

When you write a c-array (or a pointer thereto), it won't use rfl::Result, but the concept will still expect the reader to work and return a rfl::Result. I think the solution would be to adapt the concept.

https://github.com/hazby2002/reflect-cpp/blob/main/include/rfl/parsing/IsReader.hpp

There are two functions where rfl::Result<T> is expected. Instead of rfl::Result<T>, let it return rfl::Result< wrap_in_rfl_array_t<T>>. wrap_in_rfl_array_t is defined such that if T is an array, then it is wrapped and if it isn't, then wrap_in_rfl_array_t<T> is just T.

[hazby2002]

@liuzicheng1987 It works but I don't think it a good solution. Especially to_basic_type, I think we should judge the condition when T is basic type, maybe we should split requires into multiple concepts and connect them by logical expressions.

[liuzicheng1987]

@hazby2002 , yes, I suppose we could do that. Let's have that as a separate issue though.

[hazby2002]

@liuzicheng1987 This PR is ready if we will do that in a separate PR.

[hazby2002]

~I realized that the reinterpret_cast I'm using technically belongs to UB, and I'm thinking about modifying it.~

When the layouts of two structs (say std::array-s and c-arrays) are the same, using reinterpret_cast between them seems to be OK, but there's no standard to back it up.

I'm not very sure if I should change it.

[liuzicheng1987]

So, in summary, this is looking very good.

I am very positively surprised that it didn't have any measurable impact on the compile time. Considering just about how much there is going on there, that's a very pleasant surprise. I was initially very concerned about this.

Yes, I agree...the Array probably needs some minor refactoring. Basically, I would use composition over inheritance here, meaning that your Array should HAVE a std_array_t instead of BEING one.

Inheritance is generally seen as something to be avoided in modern programming and inheriting from something that isn't meant to be inherited (like std::array) is not considered good practice.

I also don't really get why you do the reinterpret_cast in the first place...you already have std::array, but then you go ahead and transform in into a c-array, which is less convenient than std::array.

There are a couple of other minor things like this, but I can clean them up myself as well. I think you did an excellent job here.

The one thing that I still have to do is to test this on MSVC. But I arrived in Taiwan two days ago, and I am still pretty jetlagged, so I should get some sleep.

I will test it on MSVC tomorrow. If that runs through (I am optimistic that it will), I will merge the current status. Whatever is then left to do in terms of cleanup, I will do myself.

Really, really great job. Thank you so much for this. This is a very cool thing to do as a first PR.

[hazby2002]

Thank you for all your help and support. Also, I'm honored to be able to contribute my codes here. It's very cool and lots of fun.

[liuzicheng1987]

@hazby2002 , I have created a special branch for this:

https://github.com/getml/reflect-cpp/tree/hazby2002-main

That's where I will resolve the remaining code beautifications. Also, some modifications were necessary because I redesigned the way the writer works.

[liuzicheng1987]

@hazby2002 , merged. Thank you so much for your contribution.

[liuzicheng1987]

@hazby2002 , do you maybe want to post in the r/cpp group on Reddit and explain how this works? I think people would be very interested in this.

[hazby2002]

@liuzicheng1987, I have never posted on Reddit before... Maybe I can try to create a post and explain my ideas.

Posted.

https://www.reddit.com/r/cpp/comments/18yyruc/reflectcpp_counting_correct_number_of_fields_in

[liuzicheng1987]

@hazby2002 , did you delete your Reddit account? I tried to message you.

[hazby2002]

@liuzicheng1987, I deleted the account that was previously associated with my gmail because the username was auto-generated and then I created a new one. I am not able to accept the chat invitation from you, with error message "Unable to join the room.", but I don't know the reason.

[liuzicheng1987]

OK…I see.