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
- Repository
- rL LLVM
Event Timeline
lib/External/isl/include/isl-noexceptions.h | ||
---|---|---|
17–18 ↗ | (On Diff #90610) | What header guard name would the version with exceptions get? (Sorry, this should maybe go into D30325) |
20–31 ↗ | (On Diff #90610) | Do we rely on some of the headers to be included implicitly (eg. isl/ctx.h)? |
116–130 ↗ | (On Diff #90610) | 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. |
130 ↗ | (On Diff #90610) | isl_ctx also has functions such as isl_ctx_get_max_operations. |
179 ↗ | (On Diff #90610) | The _val suffix is type-related which we usually prune from the method name. There's also no other get_constant. |
180 ↗ | (On Diff #90610) | Returns bool, not boolean? |
212–217 ↗ | (On Diff #90610) | Nice overloads |
218 ↗ | (On Diff #90610) | 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. |
220 ↗ | (On Diff #90610) | A dump() method would be appreciated. |
508–509 ↗ | (On Diff #90610) | isl_map_lexmin_pw_multi_aff seems to be not exposed. |
780 ↗ | (On Diff #90610) | The method is called get_str, but the isl function is pw_multi_aff_to_str . Will Sven object? |
982–983 ↗ | (On Diff #90610) | You had nice /* implicit */ markers somewhere else, but not here? |
1317 ↗ | (On Diff #90610) | Why did isl_val_int_from_si become a constructor, but isl_val_int_from_ui just disappeared? |
1399–1406 ↗ | (On Diff #90610) | How did you decide when to make a constructor? Is there a special case for zero_on_domain and val_on_domain? |
1407 ↗ | (On Diff #90610) | const std::string &str |
1668 ↗ | (On Diff #90610) | The char* returned by isl_ast_expr_to_C_str has to be free'd. |
1779–1780 ↗ | (On Diff #90610) | Nit: Space between definitions missing |
2384–2392 ↗ | (On Diff #90610) | For completeness: I'd prefer iterators over callback functions. |
3710 ↗ | (On Diff #90610) | It would save one allocation if using isl_set_to_str directly instead of the get_str() |
4514 ↗ | (On Diff #90610) | 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 | ||
---|---|---|
17–18 ↗ | (On Diff #90610) | I renamed this (in D30325) to ISL_CPP_NOEXCEPTIONS. |
20–31 ↗ | (On Diff #90610) | 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. |
116–130 ↗ | (On Diff #90610) | 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. |
130 ↗ | (On Diff #90610) | Sure, but there seems to be no need to expose them for now. |
179 ↗ | (On Diff #90610) | 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. |
180 ↗ | (On Diff #90610) | Changed. |
212–217 ↗ | (On Diff #90610) | Thanks for the positive feedback! |
218 ↗ | (On Diff #90610) | 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. |
220 ↗ | (On Diff #90610) | Added a dump method (Polly only for now). |
508–509 ↗ | (On Diff #90610) | 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. |
982–983 ↗ | (On Diff #90610) | Added in D30325. |
1317 ↗ | (On Diff #90610) | 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. |
1399–1406 ↗ | (On Diff #90610) | 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. |
1407 ↗ | (On Diff #90610) | String arguments are now passed as const std::string &str. |
1668 ↗ | (On Diff #90610) | We now properly free the char* returned by *_to_C_str. |
1779–1780 ↗ | (On Diff #90610) | Added spaces between constructors. |
2384–2392 ↗ | (On Diff #90610) | I understand (and likely agree with this). I won't include these foreach functions in the patches to upstream. |
3710 ↗ | (On Diff #90610) | Changed. |
4514 ↗ | (On Diff #90610) | 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 | ||
---|---|---|
508–509 ↗ | (On Diff #90610) | 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. |