The isl C++ binding method interface introduces a thin C++ layer that allows
to call isl methods directly on the memory managed C++ objects. This makes the
relevant methods directly available via code-completion interfaces, allows for
the use of overloading, conversion constructors, and many other nice C++
features that make using isl a lot easier.
Details
Diff Detail
- Build Status
Buildable 4677 Build 4677: arc lint + arc unit
Event Timeline
lib/External/isl/include/isl-noexceptions.h | ||
---|---|---|
16–18 | What header guard name would the version with exceptions get? (Sorry, this should maybe go into D30325) | |
20–31 | Do we rely on some of the headers to be included implicitly (eg. isl/ctx.h)? | |
117–131 | For completeness: This is a non-owning ctx class, which IMHO is the right thing to do. Will we also get an owning version of it such that we can do: { isl::owning_ctx temporary; auto map = isl::map(temporary,"{ [i] -> [2i] }"); ... } a pattern very common in the unittests. | |
131 | isl_ctx also has functions such as isl_ctx_get_max_operations. | |
180 | The _val suffix is type-related which we usually prune from the method name. There's also no other get_constant. | |
181 | Returns bool, not boolean? | |
213–218 | Nice overloads | |
219 | This one stands out very differently. The dominant creation pattern is the constructor, not named static constructors. I would generally prefer this. Eg. isl::basic_map::from_string(ctx,std::string) or isl::ctx::space(unsigned,unsigned,unsigned). I don't think we had a discussion about it yet. | |
221 | A dump() method would be appreciated. | |
509–510 | isl_map_lexmin_pw_multi_aff seems to be not exposed. | |
781–788 | The method is called get_str, but the isl function is pw_multi_aff_to_str . Will Sven object? | |
981–986 | You had nice /* implicit */ markers somewhere else, but not here? | |
1316 | Why did isl_val_int_from_si become a constructor, but isl_val_int_from_ui just disappeared? | |
1400–1407 | How did you decide when to make a constructor? Is there a special case for zero_on_domain and val_on_domain? | |
1408 | const std::string &str | |
1674 | The char* returned by isl_ast_expr_to_C_str has to be free'd. | |
1790–1791 | Nit: Space between definitions missing | |
2402–2410 | For completeness: I'd prefer iterators over callback functions. | |
3754 | It would save one allocation if using isl_set_to_str directly instead of the get_str() | |
4566 | How did you decide when to make a constructor? |
Hi Michael,
thank you for your comments. I replied to them inline and incorporated the necessary changes in the new patch, which I push in a second.
lib/External/isl/include/isl-noexceptions.h | ||
---|---|---|
16–18 | I renamed this (in D30325) to ISL_CPP_NOEXCEPTIONS. | |
20–31 | Yes. These are the files that were used to generate the bindings. They are known to be sufficient to provide all types in function signatures used in these bindings. | |
117–131 | I would like to get an owning class as well, but we can probably add it later. I will add the intentions to add such a class to the commit message that I propose to isl-dev. | |
131 | Sure, but there seems to be no need to expose them for now. | |
180 | Right. This function is only exposed in Polly. I would leave it for now as it is and submit a corresponding change to isl when exposing it officially. | |
181 | Changed. | |
213–218 | Thanks for the positive feedback! | |
219 | This function is annotated in isl as isl_export, not isl_constructor. Consequently I create a named constructor. Are you saying you prefer to not have unnamed constructors, but generally use named constructors for exported isl methods? If you do, this seems to be a change of what Sven did in the python bindings. In this case, we should probably add this as a comment in the commit message and have a discussion with Sven involved. | |
221 | Added a dump method (Polly only for now). | |
509–510 | Right. It is also not exposed in isl. In fact, I will try to exclude them from the isl python bindings I submit for review and add a note that we discuss the right return type of these functions with Sven in the code review. | |
981–986 | Added in D30325. | |
1316 | isl_val_int_from_si is today already exposed by isl as unnamed constructor, whereas isl_val_int_from_ui is not exposed and I also did not expose it myself for Polly. I exposed it now for Polly as another unnamed constructor, such that we have now constructors for both. | |
1400–1407 | These are not exposed in isl at the moment, but I added the appropriate markers isl_polly_constructor myself. I believe these are simple enough that they might not need a named constructor, but I am happy to change this. As these are not yet exposed in isl, I expect these not be part of the first bindings I submit. Hence, we can discuss these specific functions at the point when we submit a patch to expose these functions in isl. | |
1408 | String arguments are now passed as const std::string &str. | |
1674 | We now properly free the char* returned by *_to_C_str. | |
1790–1791 | Added spaces between constructors. | |
2402–2410 | I understand (and likely agree with this). I won't include these foreach functions in the patches to upstream. | |
3754 | Changed. | |
4566 | For the python bindings isl has specific markers __isl_constructor to decide which functions should become constructors. I translated precisely these to unnamed constructor and all others to named static functions. This matches the behavior of isl's current python bindings. For functions not yet exposed, I marked what seemed obvious methods as constructors and left all others as named static functions. |
lib/External/isl/include/isl-noexceptions.h | ||
---|---|---|
509–510 | I perceive isl_map_lexmin_pw_multi_aff as the more canonical variant. pw_multi_aff is implicitly convertible to map hence should make no difference in C++ if lexmin() returns pw_multi_aff instead of map. Without isl_map_lexmin_pw_multi_aff, one has to use isl_pw_multi_aff_from_map as an extra step. |
What header guard name would the version with exceptions get? (Sorry, this should maybe go into D30325)