isl_basic_set_dup no longer in headers

[Mike Frysinger]

the isl_basic_set_dup function was moved to internal headers in commit 5cd681c8d9d49ec84ccf0367346b4397d18f40c6, but the symbol is still exported, so people can still access it

i noticed because cloog is still using this function.  is duplicating a variable really considered an internal function ?  in general, it seems like basic functionality to me ...  otherwise, what is the suggested route for cloog to take here ?

-mike

[Sven Verdoolaege]

On Tue, Jul 14, 2015 at 08:31:27PM -0700, Mike Frysinger wrote:
> the isl_basic_set_dup function was moved to internal headers in commit
> 5cd681c8d9d49ec84ccf0367346b4397d18f40c6, but the symbol is still exported,
> so people can still access it
>
> i noticed because cloog is still using this function. is duplicating a
> variable really considered an internal function ?

Yes. A user shouldn't make any assumptions about the internal representation
and therefore shouldn't care how a copy is made.
In general, a user should only use functions that are documented
in the mannual.

> in general, it seems
> like basic functionality to me ... otherwise, what is the suggested route
> for cloog to take here ?

If CLooG still depends on getting a deep copy, then that should be fixed.
In principle, it shouldn't be possible for CLooG to depend on that, but
it may still be using other undocumented functions or there may be bugs
in isl. In any case, it's certainly possible to write an AST generator
using only documented functions. The one that comes with isl does that. :-)

(Note that back when the isl backend for CLooG was written, isl didn't
have any form of documentation or even a proper API, so some corners
were cut.)

skimo

[Mike Frysinger]

On Wed, Jul 15, 2015 at 3:41 PM, Sven Verdoolaege wrote:
> On Tue, Jul 14, 2015 at 08:31:27PM -0700, Mike Frysinger wrote:
>> the isl_basic_set_dup function was moved to internal headers in commit
>> 5cd681c8d9d49ec84ccf0367346b4397d18f40c6, but the symbol is still exported,
>> so people can still access it
>>
>> i noticed because cloog is still using this function. is duplicating a
>> variable really considered an internal function ?
>
> Yes. A user shouldn't make any assumptions about the internal representation
> and therefore shouldn't care how a copy is made.

i'm not sure how this statement is logically consistent. i agree
entirely that people shouldn't make assumptions about internal
representations, but isn't that exactly what the dup related functions
are designed for ? it's not like you can reasonably say this would do
the right thing:
isl_basic_set *bset;
... create/initialize bset to some state ...
isl_basic_set *tempbset = bset;
... do some operations on tempbset ...
// bset should be unmodified here but of course is not.
return bset;

with the dup api, this is now trivial:
isl_basic_set *bset;
... create/initialize bset to some state ...
isl_basic_set *tempbset = isl_basic_set_dup(bset);
... do some operations on tempbset ...
// bset is unmodified here.
return bset;

otherwise you need to open code using basic primitives like create/foreach/add.

> In general, a user should only use functions that are documented
> in the mannual.

sure, but this is why we have symbol visibility & linker versioning
scripts -- we can enforce that the only symbols exported in the ABI
are the ones that are part of the documented API. i'm saying the dup
symbols currently get exported when they should not be.

>> in general, it seems
>> like basic functionality to me ... otherwise, what is the suggested route
>> for cloog to take here ?
>
> If CLooG still depends on getting a deep copy, then that should be fixed.
> In principle, it shouldn't be possible for CLooG to depend on that, but
> it may still be using other undocumented functions or there may be bugs
> in isl. In any case, it's certainly possible to write an AST generator
> using only documented functions. The one that comes with isl does that. :-)

cloog has an api for creating a copy of a set. it is built upon the
isl_basic_set_dup function. it seems silly to require people to code
this themselves when it's (imo) a fairly basic operation people want.
the fact that isl itself has already implemented these seems to
support this position :).
-mike

[Tobias Grosser]

Did you look at isl_basic_set_copy()?

Best,
Tobias

[Sven Verdoolaege]

On Wed, Jul 15, 2015 at 04:13:58PM +0800, Mike Frysinger wrote:
> On Wed, Jul 15, 2015 at 3:41 PM, Sven Verdoolaege wrote:
> > On Tue, Jul 14, 2015 at 08:31:27PM -0700, Mike Frysinger wrote:
> >> the isl_basic_set_dup function was moved to internal headers in commit
> >> 5cd681c8d9d49ec84ccf0367346b4397d18f40c6, but the symbol is still exported,
> >> so people can still access it
> >>
> >> i noticed because cloog is still using this function. is duplicating a
> >> variable really considered an internal function ?
> >
> > Yes. A user shouldn't make any assumptions about the internal representation
> > and therefore shouldn't care how a copy is made.
>
> i'm not sure how this statement is logically consistent. i agree
> entirely that people shouldn't make assumptions about internal
> representations, but isn't that exactly what the dup related functions
> are designed for ?

I don't think you know what dup does and you shouldn't even know
(unless you want to help out in the development of isl itself).
It's a purely internal function that ended up in a public header by mistake.

> it's not like you can reasonably say this would do
> the right thing:
> isl_basic_set *bset;
> ... create/initialize bset to some state ...
> isl_basic_set *tempbset = bset;
> ... do some operations on tempbset ...
> // bset should be unmodified here but of course is not.
> return bset;
>
> with the dup api, this is now trivial:
> isl_basic_set *bset;
> ... create/initialize bset to some state ...
> isl_basic_set *tempbset = isl_basic_set_dup(bset);
> ... do some operations on tempbset ...
> // bset is unmodified here.
> return bset;

I don't know what you think dup does, but is sounds like you want
to use isl_basic_set_copy here.

skimo

[Mike Frysinger]

On Wed, Jul 15, 2015 at 4:22 PM, Sven Verdoolaege wrote:
> On Wed, Jul 15, 2015 at 04:13:58PM +0800, Mike Frysinger wrote:
>> On Wed, Jul 15, 2015 at 3:41 PM, Sven Verdoolaege wrote:
>> > On Tue, Jul 14, 2015 at 08:31:27PM -0700, Mike Frysinger wrote:
>> >> the isl_basic_set_dup function was moved to internal headers in commit
>> >> 5cd681c8d9d49ec84ccf0367346b4397d18f40c6, but the symbol is still exported,
>> >> so people can still access it
>> >>
>> >> i noticed because cloog is still using this function. is duplicating a
>> >> variable really considered an internal function ?
>> >
>> > Yes. A user shouldn't make any assumptions about the internal representation
>> > and therefore shouldn't care how a copy is made.
>>
>> i'm not sure how this statement is logically consistent. i agree
>> entirely that people shouldn't make assumptions about internal
>> representations, but isn't that exactly what the dup related functions
>> are designed for ?
>
> I don't think you know what dup does and you shouldn't even know
> (unless you want to help out in the development of isl itself).
> It's a purely internal function that ended up in a public header by mistake.

i assumed it was for duplicating the set (akin to strdup). i'm not
familiar with isl or cloog apis, just looking at the functions cloog
is using.

>> it's not like you can reasonably say this would do
>> the right thing:
>> isl_basic_set *bset;
>> ... create/initialize bset to some state ...
>> isl_basic_set *tempbset = bset;
>> ... do some operations on tempbset ...
>> // bset should be unmodified here but of course is not.
>> return bset;
>>
>> with the dup api, this is now trivial:
>> isl_basic_set *bset;
>> ... create/initialize bset to some state ...
>> isl_basic_set *tempbset = isl_basic_set_dup(bset);
>> ... do some operations on tempbset ...
>> // bset is unmodified here.
>> return bset;
>
> I don't know what you think dup does, but is sounds like you want
> to use isl_basic_set_copy here.

thanks, that sounds like it should do what i need
-mike