-
Notifications
You must be signed in to change notification settings - Fork 2
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
cpp: export isl_id #8
Comments
@TobiG I wrote it. The problem with the userpointer is that isl may need to destroy it internally after the C++ id object goes out of scope. For example, an
My understanding that We considered an alternative encoding where user data is stored in a hashmap |
The |
Interesting, I came up with the following "approach" for type safety:
which is used as follows:
The idea is that we can use the address of a type specific static global to uniquely identify the type of the content and verify if the isl::id used to read the type is indeed of the same type as the isl::id used to store the type. The above snippets are still broken as the isl pointer still references the isl id object, instead of an object that can be freed when the isl_id is destroyed. But the idea itself might be useful. |
@ftynse : Yes, it would be good to have a pull request to comment on. |
@TobiG cool trick. I think it can be combined rather easily with what we have. I did not want to have a class template for the id type itself because it could prevent me from having an (stl-style) list of ids, which expects instances of the same class. Particularly, the size of We can hide this under the template function |
Sounds great. Yes, I think the two ideas are quite complementary and can easily be combined. I like your non-templated class as this also avoids the need to return an isl_id in get_id() functions and later cast it to something else. How did you generate this class. Do you have patches to the isl C++ generator? |
So far it is hand written in |
I was mostly interested how you did it. I think starting off from a class that is defined in isl.h.top is a good idea. We can then still see if we want to automate some of the generation. It might indeed not be needed. |
More discussion: |
I don't understand. Do you want that the pointer addresses of two isl::ids compare equal if their underlying isl_ids compare equal. Or do you want them to compare equal for std::set and Co. I think in this case, we should overload std::less or operator<, no? |
isl_ctx *ctx = isl_ctx_alloc();
int i = 42;
isl_id *id1 = isl_id_alloc(ctx, "name", &i);
isl_id *id2 = isl_id_alloc(ctx, "name", &i);
assert(id1 == id2); // this holds! it would make sense to have the same behavior in C++ isl::ctx ctx(isl_ctx_alloc());
int i = 42;
isl::id id1(ctx, "name", &i);
isl::id id2(ctx, "name", &i);
assert(id1 == id2); // expect to hold
assert(id1.get() == id2.get()); // isl should also know that these ids are the same. Because we do not put a raw pointer to a C++ object, but a wrapper we constructed internally, isl will be unable to match user pointers. We have to keep track of the internally-constructed wrapper objects. |
I see. In this case, I don't think we can reasonably piggy-pack anything to the user pointer. I also don't believe that keeping a separate map in the C++ interface is the right solution. Instead, I would likely add an additional field to isl's isl_id struct and add an interface that allows the C++ bindings to officially store the data it needs. |
I don't see how an extra pointer would help. My user pointer-packing magic is intended to accept std::function as a deleter instead of a C function pointer. We cannot decay an std::function to a function pointer, so we store it instead and provide a deleter function that recovers the std::function and calls it. An alternative would be keep C function pointer as a deleter in C++ interface. At this point, the id can be treated just like any other class. |
You could do your very same packing and pass the pointer you pass today in the extra pointer and the original pointer as key in today's pointer field, could you? |
Regarding the alternative. I think it might make sense to start with the most simple solution that matches the C interface as closely as possible. Could we just export isl_id with __isl_export and then add the missing functionality to the generator to generate code for it as it is (or write the code that would be generated by hand). In the end we want the C and C++ interface to be reasonably similar. |
Well, the easiest solution would be to force the deleter in the C++ interface to be a C function pointer. |
Otherwise, having two user pointers in the C isl_id seems weird... |
But I still don't see how that would help. It would either break pointer-comparability of ids in C, or require the second pointers to be the same to get you the same id object, which brings us back to the original problem: maintain a mapping between actual user pointers aka usr1 and extra data created for that pointer aka usr2 in order to create identical pointers. |
@TobiG @nicolasvasilache We may want to keep https://github.com/nicolasvasilache/isl/pull/18 and propose https://github.com/nicolasvasilache/isl/pull/17 in a documentation as a way for the user of islpp library to have arbitrarily-typed deleters and preserve the |
As discussed face-to-face, I am largely in favor of the lightweight version.
As long as we let lifetime management happen in C I do not see why we'd
want to allow the deleter to capture.
…On Fri, Aug 4, 2017 at 12:37 PM, ftynse ***@***.***> wrote:
@TobiG <https://github.com/tobig> @nicolasvasilache
<https://github.com/nicolasvasilache>
So here are my two propositions nicolasvasilache/isl#17
<https://github.com/nicolasvasilache/isl/pull/17> nicolasvasilache/isl#18
<https://github.com/nicolasvasilache/isl/pull/18>.
We may want to keep nicolasvasilache/isl#18
<https://github.com/nicolasvasilache/isl/pull/18> and propose
nicolasvasilache/isl#17 <https://github.com/nicolasvasilache/isl/pull/17>
in a documentation as a way for the user of islpp library to have
arbitrarily-typed deleters and preserve the isl_id pointer-comparability.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#8 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AJradGq-OD_rOJrSjrOuj5vbeh-ACiY8ks5sUvRggaJpZM4OqEOI>
.
|
@ftynse , I also believe the lightweight version is preferable for now. Would you be interested in preparing a patch that is upstreamable? It seems the code is very close already. I would suggest to auto-generate the code, as this will make the patch a lot smaller and would raise less concerns during a patch review. You can likely just:
I would probably start without additional comparison operators or constructors and would for now also not move the deleter into the constructor. I would possibly even not template the code for now. While this removes some of the features we (mostly you) came up with, it is likely a very good start and very uncontroversial. |
For #11 having isl_id exported would be useful. |
Exporting isl_id is non-trivial as it takes a void* user pointer. I wonder if there are better ways to export this function than just exposing the userpointer?
@nicolasvasilache : Your private branch has a managed_isl branch and I can see some templated version of isl_id which you developed. Any idea who wrote the code, if this is automated and if this is something we could upstream.
This currently blocks me from exposing most of the list stuff.
The text was updated successfully, but these errors were encountered: