Re: [DRMAA-WG] DRMAAv2 C binding - Final Draft
Am 11.05.2012 um 16:27 schrieb Klauck, Stefan:
Hi Daniel
On May 11, 2012, at 3:17 PM, Daniel Gruber wrote:
Hi Stefan,
Am 11.05.2012 um 14:49 schrieb Klauck, Stefan:
Hello,
I put my comments below.
Best regards Stefan
On May 9, 2012, at 4:47 PM, Daniel Gruber wrote:
For the current public comment period I've following issues:
295: drmaa2_list_get(.., int pos) -> pos should be const, the implementation does not need to change it - 297 int pos -> same
The 'const' is not really necessary in the header file since the implementation file can add the const, too.
The following example is legal c code:
//foo.h void foo(int bar);
//foo.c void foo(const int bar);
But you will loose the advantage of pushing a const into the function.
Do you mean that you do not force the implementation to use a const?
Now I'm lost :) Yes, I want to have the implementations that they are forced to use the const. Hence I wan't to have this in the spec like the other consts there. This would allow DRMAA application implementers to pass a const. Having const in the innermost functions is always a good idea in order to avoid bad casts.
You could also avoid "bar" in the header (like void foo(int)), but this makes code hard to maintain. Since we have already "const" in the spec, in my eyes it would look in my eyes more consistent.
I agree. My point was that it is not really necessary to do not allow the implementation to change the 'pos' value since the value is not returned anyway. Nevertheless, the implementation could add the 'const' for optimization…
You're right, now we can force them to do so ;)
- 549 char **substate -> the user has to free it with "drmaa2_string_free()". Since it is just a string the user might "forget" this and use a "normal" free.
In the current mock implementation https://github.com/troeger/drmaav2-mock/blob/master/drmaa2.c drmaa2_string_free() is a normal free(), so that this would be ok (except for the NULL'ing).
Indeed. Here it wouldn't be a problem.
Hence I would recommend to have a drmaa2_string type because all other data (which has to be freed) is drmaa2 typed.
Sounds nevertheless reasonable.
General question: I would like to have the free functions to NULL the freed pointers (in order to minimize the risk with dangling pointers). Hence I would change *all* free methods to accept ** arguments. Wouldn't this be reasonable? Why should the user always do the NULL'ing itself? He might forget or be just lazy and running later in problems, which we could avoid easily.
Example: drmaa2_list_free(&list); /* some code later, I want to do something with list again, but not sure if it was freed already */ ... if (list == NULL) { /* oh it was freed already */ } else { /* not feed */ }
I had no opinion. Hence I googled the problem. There are many conversations concerning this topic especially on stackoverflow e.g. http://stackoverflow.com/questions/1879550/should-one-really-set-pointers-to... .
Since the "NULL'ing" is controversial, I suggest to do not set the pointer to NULL within the implementation. Applications can still implement macros or wrapper functions (for NULL'ing) to write portable code, in case they want to set freed pointers to NULL and rely on that.
Can't see any valid argument against, sorry could read the full article in all details. I guess we've to vote :)
You lose the address of the freed pointer. (That's why I would let the application developer to decide.)
example:
drmaa2_list list_reference = list; drmaa2_list_free(&list); if (list == list_reference) // false
Uhh, wasn't aware of this case (sorry, my mistake I wanted to say "couldn't read the full article in all details)...). Is such a reference to an pointer we've really needed in DRMAA apps? I'm wondering what would s.o. could do with this reference pointing to some (most likely) bad memory location. IMHO using a pointer after freeing is nothing s.o. should do by choice. Hence immediately after the list_free() the reference (if it is really needed) should be NULLed as well. Anybody who really wants to do that? Roger? Andre? Cheers, Daniel
Even though list and list_refernce are invalid pointers, it is not really intuitive that the comparison evaluates to 'false'.
In addition "if (list_reference == NULL)" is still 'false'
Cheers Stefan
Cheers,
Daniel
Thanks,
Daniel
Am 17.04.2012 um 11:16 schrieb Peter Tröger:
Dear all,
the DRMAAv2 C binding is now in final draft state. Attached you can find the annotated and the official version of the specification, as well as the raw header file.
Please provide your final comments on the mailing list until *April 22nd* (this Sunday). In case, we will set up a conf call for last discussions. Otherwise, the document will enter the OGF document process on next Monday.
Thanks and best regards, Peter.
<drmaa-c-finaldraft-annotated.pdf><drmaa-c-finaldraft-official.pdf><drmaa2.h>-- drmaa-wg mailing list drmaa-wg@ogf.org<mailto:drmaa-wg@ogf.org> https://www.ogf.org/mailman/listinfo/drmaa-wg
The main argument against NULLing the pointer is really just principle of least astonishment -- routines in C don't typically do that, especially not in OS libraries. Because there's such a large body of function calls that don't do it, throwing in some that do, can make your code confusing. Did you forget to NULL that pointer, or was it done in the library? NULLing the pointer is more practical, but only if everyone does it that way. I would vote to conform to what the majority of libraries already do. (Yes, I really did just respond to a DRMAA email!) Daniel On 5/11/12 7:51 AM, Daniel Gruber wrote:
Am 11.05.2012 um 16:27 schrieb Klauck, Stefan:
Hi Daniel
On May 11, 2012, at 3:17 PM, Daniel Gruber wrote:
Hi Stefan,
Am 11.05.2012 um 14:49 schrieb Klauck, Stefan:
Hello,
I put my comments below.
Best regards Stefan
On May 9, 2012, at 4:47 PM, Daniel Gruber wrote:
For the current public comment period I've following issues:
295: drmaa2_list_get(.., int pos) -> pos should be const, the implementation does not need to change it - 297 int pos -> same
The 'const' is not really necessary in the header file since the implementation file can add the const, too.
The following example is legal c code:
//foo.h void foo(int bar);
//foo.c void foo(const int bar);
But you will loose the advantage of pushing a const into the function. Do you mean that you do not force the implementation to use a const? Now I'm lost :) Yes, I want to have the implementations that they are forced to use the const. Hence I wan't to have this in the spec like the other consts there. This would allow DRMAA application implementers to pass a const. Having const in the innermost functions is always a good idea in order to avoid bad casts.
You could also avoid "bar" in the header (like void foo(int)), but this makes code hard to maintain. Since we have already "const" in the spec, in my eyes it would look in my eyes more consistent.
I agree. My point was that it is not really necessary to do not allow the implementation to change the 'pos' value since the value is not returned anyway. Nevertheless, the implementation could add the 'const' for optimization… You're right, now we can force them to do so ;)
- 549 char **substate -> the user has to free it with "drmaa2_string_free()". Since it is just a string the user might "forget" this and use a "normal" free.
In the current mock implementation https://github.com/troeger/drmaav2-mock/blob/master/drmaa2.c drmaa2_string_free() is a normal free(), so that this would be ok (except for the NULL'ing).
Indeed. Here it wouldn't be a problem.
Hence I would recommend to have a drmaa2_string type because all other data (which has to be freed) is drmaa2 typed.
Sounds nevertheless reasonable.
General question: I would like to have the free functions to NULL the freed pointers (in order to minimize the risk with dangling pointers). Hence I would change *all* free methods to accept ** arguments. Wouldn't this be reasonable? Why should the user always do the NULL'ing itself? He might forget or be just lazy and running later in problems, which we could avoid easily.
Example: drmaa2_list_free(&list); /* some code later, I want to do something with list again, but not sure if it was freed already */ ... if (list == NULL) { /* oh it was freed already */ } else { /* not feed */ }
I had no opinion. Hence I googled the problem. There are many conversations concerning this topic especially on stackoverflow e.g. http://stackoverflow.com/questions/1879550/should-one-really-set-pointers-to... .
Since the "NULL'ing" is controversial, I suggest to do not set the pointer to NULL within the implementation. Applications can still implement macros or wrapper functions (for NULL'ing) to write portable code, in case they want to set freed pointers to NULL and rely on that. Can't see any valid argument against, sorry could read the full article in all details. I guess we've to vote :) You lose the address of the freed pointer. (That's why I would let the application developer to decide.)
example:
drmaa2_list list_reference = list; drmaa2_list_free(&list); if (list == list_reference) // false Uhh, wasn't aware of this case (sorry, my mistake I wanted to say "couldn't read the full article in all details)...). Is such a reference to an pointer we've really needed in DRMAA apps? I'm wondering what would s.o. could do with this reference pointing to some (most likely) bad memory location. IMHO using a pointer after freeing is nothing s.o. should do by choice. Hence immediately after the list_free() the reference (if it is really needed) should be NULLed as well.
Anybody who really wants to do that? Roger? Andre?
Cheers,
Daniel
Even though list and list_refernce are invalid pointers, it is not really intuitive that the comparison evaluates to 'false'.
In addition "if (list_reference == NULL)" is still 'false'
Cheers Stefan
Cheers,
Daniel
Thanks,
Daniel
Am 17.04.2012 um 11:16 schrieb Peter Tröger:
Dear all,
the DRMAAv2 C binding is now in final draft state. Attached you can find the annotated and the official version of the specification, as well as the raw header file.
Please provide your final comments on the mailing list until *April 22nd* (this Sunday). In case, we will set up a conf call for last discussions. Otherwise, the document will enter the OGF document process on next Monday.
Thanks and best regards, Peter.
<drmaa-c-finaldraft-annotated.pdf><drmaa-c-finaldraft-official.pdf><drmaa2.h>-- drmaa-wg mailing list drmaa-wg@ogf.org<mailto:drmaa-wg@ogf.org> https://www.ogf.org/mailman/listinfo/drmaa-wg
-- drmaa-wg mailing list drmaa-wg@ogf.org https://www.ogf.org/mailman/listinfo/drmaa-wg
-- Get Apache Hadoop for the Enterprise: http://www.cloudera.com/downloads/ Follow us @cloudera or http://www.facebook.com/cloudera
Hi Dan - welcome back! I also think that it is more consistent to not store NULL a pointer. That interface is way easier for normal programmers to understand as the most common memory allocation routines are malloc() & free(). Rayson ================================= Open Grid Scheduler / Grid Engine http://gridscheduler.sourceforge.net/ Scalable Grid Engine Support Program http://www.scalablelogic.com/ On Fri, May 11, 2012 at 12:47 PM, Daniel Templeton <daniel@cloudera.com> wrote:
The main argument against NULLing the pointer is really just principle of least astonishment -- routines in C don't typically do that, especially not in OS libraries. Because there's such a large body of function calls that don't do it, throwing in some that do, can make your code confusing. Did you forget to NULL that pointer, or was it done in the library? NULLing the pointer is more practical, but only if everyone does it that way. I would vote to conform to what the majority of libraries already do.
(Yes, I really did just respond to a DRMAA email!)
Daniel
On 5/11/12 7:51 AM, Daniel Gruber wrote:
Am 11.05.2012 um 16:27 schrieb Klauck, Stefan:
Hi Daniel
On May 11, 2012, at 3:17 PM, Daniel Gruber wrote:
Hi Stefan,
Am 11.05.2012 um 14:49 schrieb Klauck, Stefan:
Hello,
I put my comments below.
Best regards Stefan
On May 9, 2012, at 4:47 PM, Daniel Gruber wrote:
For the current public comment period I've following issues:
295: drmaa2_list_get(.., int pos) -> pos should be const, the implementation does not need to change it - 297 int pos -> same
The 'const' is not really necessary in the header file since the implementation file can add the const, too.
The following example is legal c code:
//foo.h void foo(int bar);
//foo.c void foo(const int bar);
But you will loose the advantage of pushing a const into the function.
Do you mean that you do not force the implementation to use a const?
Now I'm lost :) Yes, I want to have the implementations that they are forced to use the const. Hence I wan't to have this in the spec like the other consts there. This would allow DRMAA application implementers to pass a const. Having const in the innermost functions is always a good idea in order to avoid bad casts.
You could also avoid "bar" in the header (like void foo(int)), but this makes code hard to maintain. Since we have already "const" in the spec, in my eyes it would look in my eyes more consistent.
I agree. My point was that it is not really necessary to do not allow the implementation to change the 'pos' value since the value is not returned anyway. Nevertheless, the implementation could add the 'const' for optimization…
You're right, now we can force them to do so ;)
- 549 char **substate -> the user has to free it with "drmaa2_string_free()". Since it is just a string the user might "forget" this and use a "normal" free.
In the current mock implementation https://github.com/troeger/drmaav2-mock/blob/master/drmaa2.c drmaa2_string_free() is a normal free(), so that this would be ok (except for the NULL'ing).
Indeed. Here it wouldn't be a problem.
Hence I would recommend to have a drmaa2_string type because all other data (which has to be freed) is drmaa2 typed.
Sounds nevertheless reasonable.
General question: I would like to have the free functions to NULL the freed pointers (in order to minimize the risk with dangling pointers). Hence I would change *all* free methods to accept ** arguments. Wouldn't this be reasonable? Why should the user always do the NULL'ing itself? He might forget or be just lazy and running later in problems, which we could avoid easily.
Example: drmaa2_list_free(&list); /* some code later, I want to do something with list again, but not sure if it was freed already */ ... if (list == NULL) { /* oh it was freed already */ } else { /* not feed */ }
I had no opinion. Hence I googled the problem. There are many conversations concerning this topic especially on stackoverflow e.g. http://stackoverflow.com/questions/1879550/should-one-really-set-pointers-to... .
Since the "NULL'ing" is controversial, I suggest to do not set the pointer to NULL within the implementation. Applications can still implement macros or wrapper functions (for NULL'ing) to write portable code, in case they want to set freed pointers to NULL and rely on that.
Can't see any valid argument against, sorry could read the full article in all details. I guess we've to vote :)
You lose the address of the freed pointer. (That's why I would let the application developer to decide.)
example:
drmaa2_list list_reference = list; drmaa2_list_free(&list); if (list == list_reference) // false
Uhh, wasn't aware of this case (sorry, my mistake I wanted to say "couldn't read the full article in all details)...). Is such a reference to an pointer we've really needed in DRMAA apps? I'm wondering what would s.o. could do with this reference pointing to some (most likely) bad memory location. IMHO using a pointer after freeing is nothing s.o. should do by choice. Hence immediately after the list_free() the reference (if it is really needed) should be NULLed as well.
Anybody who really wants to do that? Roger? Andre?
Cheers,
Daniel
Even though list and list_refernce are invalid pointers, it is not really intuitive that the comparison evaluates to 'false'.
In addition "if (list_reference == NULL)" is still 'false'
Cheers Stefan
Cheers,
Daniel
Thanks,
Daniel
Am 17.04.2012 um 11:16 schrieb Peter Tröger:
Dear all,
the DRMAAv2 C binding is now in final draft state. Attached you can find the annotated and the official version of the specification, as well as the raw header file.
Please provide your final comments on the mailing list until *April 22nd* (this Sunday). In case, we will set up a conf call for last discussions. Otherwise, the document will enter the OGF document process on next Monday.
Thanks and best regards, Peter.
<drmaa-c-finaldraft-annotated.pdf><drmaa-c-finaldraft-official.pdf><drmaa2.h>-- drmaa-wg mailing list drmaa-wg@ogf.org<mailto:drmaa-wg@ogf.org> https://www.ogf.org/mailman/listinfo/drmaa-wg
-- drmaa-wg mailing list drmaa-wg@ogf.org https://www.ogf.org/mailman/listinfo/drmaa-wg
-- Get Apache Hadoop for the Enterprise: http://www.cloudera.com/downloads/ Follow us @cloudera or http://www.facebook.com/cloudera
-- drmaa-wg mailing list drmaa-wg@ogf.org https://www.ogf.org/mailman/listinfo/drmaa-wg
Hi Rayson, Just to make sure I understand you. Do you mean..."It is more consistent not to NULL pointers" because C programmers typically know that they should handle this themselves? Regards, Bill. On 2012-05-11, at 1:34 PM, Rayson Ho wrote:
Hi Dan - welcome back!
I also think that it is more consistent to not store NULL a pointer.
That interface is way easier for normal programmers to understand as the most common memory allocation routines are malloc() & free().
Rayson
================================= Open Grid Scheduler / Grid Engine http://gridscheduler.sourceforge.net/
Scalable Grid Engine Support Program http://www.scalablelogic.com/
On Fri, May 11, 2012 at 12:47 PM, Daniel Templeton <daniel@cloudera.com> wrote:
The main argument against NULLing the pointer is really just principle of least astonishment -- routines in C don't typically do that, especially not in OS libraries. Because there's such a large body of function calls that don't do it, throwing in some that do, can make your code confusing. Did you forget to NULL that pointer, or was it done in the library? NULLing the pointer is more practical, but only if everyone does it that way. I would vote to conform to what the majority of libraries already do.
(Yes, I really did just respond to a DRMAA email!)
Daniel
On 5/11/12 7:51 AM, Daniel Gruber wrote:
Am 11.05.2012 um 16:27 schrieb Klauck, Stefan:
Hi Daniel
On May 11, 2012, at 3:17 PM, Daniel Gruber wrote:
Hi Stefan,
Am 11.05.2012 um 14:49 schrieb Klauck, Stefan:
Hello,
I put my comments below.
Best regards Stefan
On May 9, 2012, at 4:47 PM, Daniel Gruber wrote:
For the current public comment period I've following issues:
295: drmaa2_list_get(.., int pos) -> pos should be const, the implementation does not need to change it - 297 int pos -> same
The 'const' is not really necessary in the header file since the implementation file can add the const, too.
The following example is legal c code:
//foo.h void foo(int bar);
//foo.c void foo(const int bar);
But you will loose the advantage of pushing a const into the function.
Do you mean that you do not force the implementation to use a const?
Now I'm lost :) Yes, I want to have the implementations that they are forced to use the const. Hence I wan't to have this in the spec like the other consts there. This would allow DRMAA application implementers to pass a const. Having const in the innermost functions is always a good idea in order to avoid bad casts.
You could also avoid "bar" in the header (like void foo(int)), but this makes code hard to maintain. Since we have already "const" in the spec, in my eyes it would look in my eyes more consistent.
I agree. My point was that it is not really necessary to do not allow the implementation to change the 'pos' value since the value is not returned anyway. Nevertheless, the implementation could add the 'const' for optimization…
You're right, now we can force them to do so ;)
- 549 char **substate -> the user has to free it with "drmaa2_string_free()". Since it is just a string the user might "forget" this and use a "normal" free.
In the current mock implementation https://github.com/troeger/drmaav2-mock/blob/master/drmaa2.c drmaa2_string_free() is a normal free(), so that this would be ok (except for the NULL'ing).
Indeed. Here it wouldn't be a problem.
Hence I would recommend to have a drmaa2_string type because all other data (which has to be freed) is drmaa2 typed.
Sounds nevertheless reasonable.
General question: I would like to have the free functions to NULL the freed pointers (in order to minimize the risk with dangling pointers). Hence I would change *all* free methods to accept ** arguments. Wouldn't this be reasonable? Why should the user always do the NULL'ing itself? He might forget or be just lazy and running later in problems, which we could avoid easily.
Example: drmaa2_list_free(&list); /* some code later, I want to do something with list again, but not sure if it was freed already */ ... if (list == NULL) { /* oh it was freed already */ } else { /* not feed */ }
I had no opinion. Hence I googled the problem. There are many conversations concerning this topic especially on stackoverflow e.g. http://stackoverflow.com/questions/1879550/should-one-really-set-pointers-to... .
Since the "NULL'ing" is controversial, I suggest to do not set the pointer to NULL within the implementation. Applications can still implement macros or wrapper functions (for NULL'ing) to write portable code, in case they want to set freed pointers to NULL and rely on that.
Can't see any valid argument against, sorry could read the full article in all details. I guess we've to vote :)
You lose the address of the freed pointer. (That's why I would let the application developer to decide.)
example:
drmaa2_list list_reference = list; drmaa2_list_free(&list); if (list == list_reference) // false
Uhh, wasn't aware of this case (sorry, my mistake I wanted to say "couldn't read the full article in all details)...). Is such a reference to an pointer we've really needed in DRMAA apps? I'm wondering what would s.o. could do with this reference pointing to some (most likely) bad memory location. IMHO using a pointer after freeing is nothing s.o. should do by choice. Hence immediately after the list_free() the reference (if it is really needed) should be NULLed as well.
Anybody who really wants to do that? Roger? Andre?
Cheers,
Daniel
Even though list and list_refernce are invalid pointers, it is not really intuitive that the comparison evaluates to 'false'.
In addition "if (list_reference == NULL)" is still 'false'
Cheers Stefan
Cheers,
Daniel
Thanks,
Daniel
Am 17.04.2012 um 11:16 schrieb Peter Tröger:
Dear all,
the DRMAAv2 C binding is now in final draft state. Attached you can find the annotated and the official version of the specification, as well as the raw header file.
Please provide your final comments on the mailing list until *April 22nd* (this Sunday). In case, we will set up a conf call for last discussions. Otherwise, the document will enter the OGF document process on next Monday.
Thanks and best regards, Peter.
<drmaa-c-finaldraft-annotated.pdf><drmaa-c-finaldraft-official.pdf><drmaa2.h>-- drmaa-wg mailing list drmaa-wg@ogf.org<mailto:drmaa-wg@ogf.org> https://www.ogf.org/mailman/listinfo/drmaa-wg
-- drmaa-wg mailing list drmaa-wg@ogf.org https://www.ogf.org/mailman/listinfo/drmaa-wg
-- Get Apache Hadoop for the Enterprise: http://www.cloudera.com/downloads/ Follow us @cloudera or http://www.facebook.com/cloudera
-- drmaa-wg mailing list drmaa-wg@ogf.org https://www.ogf.org/mailman/listinfo/drmaa-wg -- drmaa-wg mailing list drmaa-wg@ogf.org https://www.ogf.org/mailman/listinfo/drmaa-wg
William Bryce | VP of Products Univa Corporation - 1001 Warrenville Road, Suite 100 Lisle, Il, 65032 USA Email bbryce@univa.com | Mobile: 512.751.8014 | Office: 416.519.2934
Hi Bill, Right, this is what C & C++ system libraries do - they don't touch the pointer variable passed into them. Examples: realloc(), free() - the pointer is just a variable that tells the library routine where to go to get the memory, but the pointer variable itself should not be changed by the library routine. Rayson ================================= Open Grid Scheduler / Grid Engine http://gridscheduler.sourceforge.net/ Scalable Grid Engine Support Program http://www.scalablelogic.com/ On Fri, May 11, 2012 at 2:28 PM, Bill Bryce <bbryce@univa.com> wrote:
Hi Rayson,
Just to make sure I understand you. Do you mean..."It is more consistent not to NULL pointers" because C programmers typically know that they should handle this themselves?
Regards,
Bill.
On 2012-05-11, at 1:34 PM, Rayson Ho wrote:
Hi Dan - welcome back!
I also think that it is more consistent to not store NULL a pointer.
That interface is way easier for normal programmers to understand as the most common memory allocation routines are malloc() & free().
Rayson
================================= Open Grid Scheduler / Grid Engine http://gridscheduler.sourceforge.net/
Scalable Grid Engine Support Program http://www.scalablelogic.com/
On Fri, May 11, 2012 at 12:47 PM, Daniel Templeton <daniel@cloudera.com> wrote:
The main argument against NULLing the pointer is really just principle of least astonishment -- routines in C don't typically do that, especially not in OS libraries. Because there's such a large body of function calls that don't do it, throwing in some that do, can make your code confusing. Did you forget to NULL that pointer, or was it done in the library? NULLing the pointer is more practical, but only if everyone does it that way. I would vote to conform to what the majority of libraries already do.
(Yes, I really did just respond to a DRMAA email!)
Daniel
On 5/11/12 7:51 AM, Daniel Gruber wrote:
Am 11.05.2012 um 16:27 schrieb Klauck, Stefan:
Hi Daniel
On May 11, 2012, at 3:17 PM, Daniel Gruber wrote:
Hi Stefan,
Am 11.05.2012 um 14:49 schrieb Klauck, Stefan:
> Hello, > > I put my comments below. > > Best regards > Stefan > > > On May 9, 2012, at 4:47 PM, Daniel Gruber wrote: > > For the current public comment period I've following issues: > > 295: drmaa2_list_get(.., int pos) -> pos should be const, the > implementation does not need to change it > - 297 int pos -> same > > The 'const' is not really necessary in the header file since the > implementation file can add the const, too. > > The following example is legal c code: > > //foo.h > void foo(int bar); > > //foo.c > void foo(const int bar); > But you will loose the advantage of pushing a const into the function.
Do you mean that you do not force the implementation to use a const?
Now I'm lost :) Yes, I want to have the implementations that they are forced to use the const. Hence I wan't to have this in the spec like the other consts there. This would allow DRMAA application implementers to pass a const. Having const in the innermost functions is always a good idea in order to avoid bad casts.
You could also avoid "bar" in the header (like void foo(int)), but this makes code hard to maintain. Since we have already "const" in the spec, in my eyes it would look in my eyes more consistent.
I agree. My point was that it is not really necessary to do not allow the implementation to change the 'pos' value since the value is not returned anyway. Nevertheless, the implementation could add the 'const' for optimization…
You're right, now we can force them to do so ;)
> > - 549 char **substate -> the user has to free it with > "drmaa2_string_free()". Since it is just a string the user might "forget" > this and use > a "normal" free. > > In the current mock implementation > https://github.com/troeger/drmaav2-mock/blob/master/drmaa2.c > drmaa2_string_free() is a normal free(), so that this would be ok (except > for the NULL'ing).
Indeed. Here it wouldn't be a problem.
> Hence I would recommend to have a drmaa2_string type because all other > data (which has to be freed) is drmaa2 typed. > > > Sounds nevertheless reasonable. > > > General question: I would like to have the free functions to NULL the > freed pointers (in order to minimize the risk with > dangling pointers). Hence I would change *all* free methods to accept > ** arguments. Wouldn't this be reasonable? > Why should the user always do the NULL'ing itself? He might forget or > be just lazy and running later in problems, which > we could avoid easily. > > Example: > drmaa2_list_free(&list); > /* some code later, I want to do something with list again, but not > sure if it was freed already */ > ... > if (list == NULL) { > /* oh it was freed already */ > } else { > /* not feed */ > } > > > I had no opinion. Hence I googled the problem. > There are many conversations concerning this topic especially on > stackoverflow > e.g. > http://stackoverflow.com/questions/1879550/should-one-really-set-pointers-to... > . > > Since the "NULL'ing" is controversial, I suggest to do not set the > pointer to NULL within the implementation. > Applications can still implement macros or wrapper functions (for > NULL'ing) to write portable code, in case they want to set freed pointers to > NULL and rely on that.
Can't see any valid argument against, sorry could read the full article in all details. I guess we've to vote :)
You lose the address of the freed pointer. (That's why I would let the application developer to decide.)
example:
drmaa2_list list_reference = list; drmaa2_list_free(&list); if (list == list_reference) // false
Uhh, wasn't aware of this case (sorry, my mistake I wanted to say "couldn't read the full article in all details)...). Is such a reference to an pointer we've really needed in DRMAA apps? I'm wondering what would s.o. could do with this reference pointing to some (most likely) bad memory location. IMHO using a pointer after freeing is nothing s.o. should do by choice. Hence immediately after the list_free() the reference (if it is really needed) should be NULLed as well.
Anybody who really wants to do that? Roger? Andre?
Cheers,
Daniel
Even though list and list_refernce are invalid pointers, it is not really intuitive that the comparison evaluates to 'false'.
In addition "if (list_reference == NULL)" is still 'false'
Cheers Stefan
Cheers,
Daniel
> Thanks, > > Daniel > > > Am 17.04.2012 um 11:16 schrieb Peter Tröger: > > Dear all, > > the DRMAAv2 C binding is now in final draft state. Attached you can > find the annotated and the official version of the specification, as well as > the raw header file. > > Please provide your final comments on the mailing list until *April > 22nd* (this Sunday). In case, we will set up a conf call for last > discussions. Otherwise, the document will enter the OGF document process on > next Monday. > > Thanks and best regards, > Peter. > > > > <drmaa-c-finaldraft-annotated.pdf><drmaa-c-finaldraft-official.pdf><drmaa2.h>-- > drmaa-wg mailing list > drmaa-wg@ogf.org<mailto:drmaa-wg@ogf.org> > https://www.ogf.org/mailman/listinfo/drmaa-wg > > -- drmaa-wg mailing list drmaa-wg@ogf.org https://www.ogf.org/mailman/listinfo/drmaa-wg
-- Get Apache Hadoop for the Enterprise: http://www.cloudera.com/downloads/ Follow us @cloudera or http://www.facebook.com/cloudera
-- drmaa-wg mailing list drmaa-wg@ogf.org https://www.ogf.org/mailman/listinfo/drmaa-wg -- drmaa-wg mailing list drmaa-wg@ogf.org https://www.ogf.org/mailman/listinfo/drmaa-wg
William Bryce | VP of Products Univa Corporation - 1001 Warrenville Road, Suite 100 Lisle, Il, 65032 USA Email bbryce@univa.com | Mobile: 512.751.8014 | Office: 416.519.2934
The main argument against NULLing the pointer is really just principle of least astonishment -- routines in C don't typically do that, especially not in OS libraries. Because there's such a large body of function calls that don't do it, throwing in some that do, can make your code confusing. Did you forget to NULL that pointer, or was it done in the library? NULLing the pointer is more practical, but only if everyone does it that way. I would vote to conform to what the majority of libraries already do.
Completely agreed - there is nothing like this in POSIX, which was the golden standard at least for DRMAAv1. Best regards, Peter.
Daniel
On 5/11/12 7:51 AM, Daniel Gruber wrote:
Am 11.05.2012 um 16:27 schrieb Klauck, Stefan:
Hi Daniel
On May 11, 2012, at 3:17 PM, Daniel Gruber wrote:
Hi Stefan,
Am 11.05.2012 um 14:49 schrieb Klauck, Stefan:
Hello,
I put my comments below.
Best regards Stefan
On May 9, 2012, at 4:47 PM, Daniel Gruber wrote:
For the current public comment period I've following issues:
295: drmaa2_list_get(.., int pos) -> pos should be const, the implementation does not need to change it - 297 int pos -> same
The 'const' is not really necessary in the header file since the implementation file can add the const, too.
The following example is legal c code:
//foo.h void foo(int bar);
//foo.c void foo(const int bar);
But you will loose the advantage of pushing a const into the function. Do you mean that you do not force the implementation to use a const? Now I'm lost :) Yes, I want to have the implementations that they are forced to use the const. Hence I wan't to have this in the spec like the other consts there. This would allow DRMAA application implementers to pass a const. Having const in the innermost functions is always a good idea in order to avoid bad casts.
You could also avoid "bar" in the header (like void foo(int)), but this makes code hard to maintain. Since we have already "const" in the spec, in my eyes it would look in my eyes more consistent.
I agree. My point was that it is not really necessary to do not allow the implementation to change the 'pos' value since the value is not returned anyway. Nevertheless, the implementation could add the 'const' for optimization… You're right, now we can force them to do so ;)
- 549 char **substate -> the user has to free it with "drmaa2_string_free()". Since it is just a string the user might "forget" this and use a "normal" free.
In the current mock implementation https://github.com/troeger/drmaav2-mock/blob/master/drmaa2.c drmaa2_string_free() is a normal free(), so that this would be ok (except for the NULL'ing).
Indeed. Here it wouldn't be a problem.
Hence I would recommend to have a drmaa2_string type because all other data (which has to be freed) is drmaa2 typed.
Sounds nevertheless reasonable.
General question: I would like to have the free functions to NULL the freed pointers (in order to minimize the risk with dangling pointers). Hence I would change *all* free methods to accept ** arguments. Wouldn't this be reasonable? Why should the user always do the NULL'ing itself? He might forget or be just lazy and running later in problems, which we could avoid easily.
Example: drmaa2_list_free(&list); /* some code later, I want to do something with list again, but not sure if it was freed already */ ... if (list == NULL) { /* oh it was freed already */ } else { /* not feed */ }
I had no opinion. Hence I googled the problem. There are many conversations concerning this topic especially on stackoverflow e.g. http://stackoverflow.com/questions/1879550/should-one-really-set-pointers-to... .
Since the "NULL'ing" is controversial, I suggest to do not set the pointer to NULL within the implementation. Applications can still implement macros or wrapper functions (for NULL'ing) to write portable code, in case they want to set freed pointers to NULL and rely on that. Can't see any valid argument against, sorry could read the full article in all details. I guess we've to vote :) You lose the address of the freed pointer. (That's why I would let the application developer to decide.)
example:
drmaa2_list list_reference = list; drmaa2_list_free(&list); if (list == list_reference) // false Uhh, wasn't aware of this case (sorry, my mistake I wanted to say "couldn't read the full article in all details)...). Is such a reference to an pointer we've really needed in DRMAA apps? I'm wondering what would s.o. could do with this reference pointing to some (most likely) bad memory location. IMHO using a pointer after freeing is nothing s.o. should do by choice. Hence immediately after the list_free() the reference (if it is really needed) should be NULLed as well.
Anybody who really wants to do that? Roger? Andre?
Cheers,
Daniel
Even though list and list_refernce are invalid pointers, it is not really intuitive that the comparison evaluates to 'false'.
In addition "if (list_reference == NULL)" is still 'false'
Cheers Stefan
Cheers,
Daniel
Thanks,
Daniel
Am 17.04.2012 um 11:16 schrieb Peter Tröger:
Dear all,
the DRMAAv2 C binding is now in final draft state. Attached you can find the annotated and the official version of the specification, as well as the raw header file.
Please provide your final comments on the mailing list until *April 22nd* (this Sunday). In case, we will set up a conf call for last discussions. Otherwise, the document will enter the OGF document process on next Monday.
Thanks and best regards, Peter.
<drmaa-c-finaldraft-annotated.pdf><drmaa-c-finaldraft-official.pdf><drmaa2.h>--
drmaa-wg mailing list drmaa-wg@ogf.org<mailto:drmaa-wg@ogf.org> https://www.ogf.org/mailman/listinfo/drmaa-wg
-- drmaa-wg mailing list drmaa-wg@ogf.org https://www.ogf.org/mailman/listinfo/drmaa-wg
I wouldn't say that a programmer would be astonished with any solution. The signature implies *unambiguously* what is going to be changed. Of course an OS free() doesn't NULL because it can't. I wouldn't compare the DRMAA obj free with traditional raw free or OS defined functions, which most likely didn't change the last decades. In popular libraries like OpenMPI (which drmaa app writer most likely use) you have the exact counter-examples: "int MPI_Comm_free(MPI_Comm *comm) Description: This operation marks the communicator object for deallocation. The handle is set to MPI_COMM_NULL." (see http://www.open-mpi.org/doc/v1.4/man3/MPI_Comm_free.3.php) Same is with all other MPI object frees (with exception of a "raw" memory free). Source of their frees looks like this: 166 /* 167 * Free an info handle and all of its keys and values. 168 */ 169 int ompi_info_free (ompi_info_t **info) 170 { 171 (*info)->i_freed = true; 172 OBJ_RELEASE(*info); 173 *info = MPI_INFO_NULL; 174 return MPI_SUCCESS; 175 } 176 from: https://svn.open-mpi.org/source/xref/ompi-trunk/ompi/info/info.c Anyway both solutions would work, but until now I couldn't find any real disadvantage... Cheers, Daniel Am 11.05.2012 um 21:18 schrieb Peter Tröger:
The main argument against NULLing the pointer is really just principle of least astonishment -- routines in C don't typically do that, especially not in OS libraries. Because there's such a large body of function calls that don't do it, throwing in some that do, can make your code confusing. Did you forget to NULL that pointer, or was it done in the library? NULLing the pointer is more practical, but only if everyone does it that way. I would vote to conform to what the majority of libraries already do.
Completely agreed - there is nothing like this in POSIX, which was the golden standard at least for DRMAAv1.
Best regards, Peter.
Daniel
On 5/11/12 7:51 AM, Daniel Gruber wrote:
Am 11.05.2012 um 16:27 schrieb Klauck, Stefan:
Hi Daniel
On May 11, 2012, at 3:17 PM, Daniel Gruber wrote:
Hi Stefan,
Am 11.05.2012 um 14:49 schrieb Klauck, Stefan:
Hello,
I put my comments below.
Best regards Stefan
On May 9, 2012, at 4:47 PM, Daniel Gruber wrote:
For the current public comment period I've following issues:
295: drmaa2_list_get(.., int pos) -> pos should be const, the implementation does not need to change it - 297 int pos -> same
The 'const' is not really necessary in the header file since the implementation file can add the const, too.
The following example is legal c code:
//foo.h void foo(int bar);
//foo.c void foo(const int bar);
But you will loose the advantage of pushing a const into the function. Do you mean that you do not force the implementation to use a const? Now I'm lost :) Yes, I want to have the implementations that they are forced to use the const. Hence I wan't to have this in the spec like the other consts there. This would allow DRMAA application implementers to pass a const. Having const in the innermost functions is always a good idea in order to avoid bad casts.
You could also avoid "bar" in the header (like void foo(int)), but this makes code hard to maintain. Since we have already "const" in the spec, in my eyes it would look in my eyes more consistent.
I agree. My point was that it is not really necessary to do not allow the implementation to change the 'pos' value since the value is not returned anyway. Nevertheless, the implementation could add the 'const' for optimization… You're right, now we can force them to do so ;)
- 549 char **substate -> the user has to free it with "drmaa2_string_free()". Since it is just a string the user might "forget" this and use a "normal" free.
In the current mock implementation https://github.com/troeger/drmaav2-mock/blob/master/drmaa2.c drmaa2_string_free() is a normal free(), so that this would be ok (except for the NULL'ing).
Indeed. Here it wouldn't be a problem.
Hence I would recommend to have a drmaa2_string type because all other data (which has to be freed) is drmaa2 typed.
Sounds nevertheless reasonable.
General question: I would like to have the free functions to NULL the freed pointers (in order to minimize the risk with dangling pointers). Hence I would change *all* free methods to accept ** arguments. Wouldn't this be reasonable? Why should the user always do the NULL'ing itself? He might forget or be just lazy and running later in problems, which we could avoid easily.
Example: drmaa2_list_free(&list); /* some code later, I want to do something with list again, but not sure if it was freed already */ ... if (list == NULL) { /* oh it was freed already */ } else { /* not feed */ }
I had no opinion. Hence I googled the problem. There are many conversations concerning this topic especially on stackoverflow e.g. http://stackoverflow.com/questions/1879550/should-one-really-set-pointers-to... .
Since the "NULL'ing" is controversial, I suggest to do not set the pointer to NULL within the implementation. Applications can still implement macros or wrapper functions (for NULL'ing) to write portable code, in case they want to set freed pointers to NULL and rely on that. Can't see any valid argument against, sorry could read the full article in all details. I guess we've to vote :) You lose the address of the freed pointer. (That's why I would let the application developer to decide.)
example:
drmaa2_list list_reference = list; drmaa2_list_free(&list); if (list == list_reference) // false Uhh, wasn't aware of this case (sorry, my mistake I wanted to say "couldn't read the full article in all details)...). Is such a reference to an pointer we've really needed in DRMAA apps? I'm wondering what would s.o. could do with this reference pointing to some (most likely) bad memory location. IMHO using a pointer after freeing is nothing s.o. should do by choice. Hence immediately after the list_free() the reference (if it is really needed) should be NULLed as well.
Anybody who really wants to do that? Roger? Andre?
Cheers,
Daniel
Even though list and list_refernce are invalid pointers, it is not really intuitive that the comparison evaluates to 'false'.
In addition "if (list_reference == NULL)" is still 'false'
Cheers Stefan
Cheers,
Daniel
Thanks,
Daniel
Am 17.04.2012 um 11:16 schrieb Peter Tröger:
Dear all,
the DRMAAv2 C binding is now in final draft state. Attached you can find the annotated and the official version of the specification, as well as the raw header file.
Please provide your final comments on the mailing list until *April 22nd* (this Sunday). In case, we will set up a conf call for last discussions. Otherwise, the document will enter the OGF document process on next Monday.
Thanks and best regards, Peter.
<drmaa-c-finaldraft-annotated.pdf><drmaa-c-finaldraft-official.pdf><drmaa2.h>--
drmaa-wg mailing list drmaa-wg@ogf.org<mailto:drmaa-wg@ogf.org> https://www.ogf.org/mailman/listinfo/drmaa-wg
-- drmaa-wg mailing list drmaa-wg@ogf.org https://www.ogf.org/mailman/listinfo/drmaa-wg
-- drmaa-wg mailing list drmaa-wg@ogf.org https://www.ogf.org/mailman/listinfo/drmaa-wg
Good example Daniel. We don't have to do what POSIX does. Regards Bill Sent from my iPad - US cell: 847-612-8966 On 2012-05-12, at 4:42 AM, Daniel Gruber <dgruber@univa.com> wrote:
I wouldn't say that a programmer would be astonished with any solution. The signature implies *unambiguously* what is going to be changed. Of course an OS free() doesn't NULL because it can't. I wouldn't compare the DRMAA obj free with traditional raw free or OS defined functions, which most likely didn't change the last decades.
In popular libraries like OpenMPI (which drmaa app writer most likely use) you have the exact counter-examples:
"int MPI_Comm_free(MPI_Comm *comm)
Description: This operation marks the communicator object for deallocation. The handle is set to MPI_COMM_NULL." (see http://www.open-mpi.org/doc/v1.4/man3/MPI_Comm_free.3.php)
Same is with all other MPI object frees (with exception of a "raw" memory free).
Source of their frees looks like this: 166 /* 167 * Free an info handle and all of its keys and values. 168 */ 169 int ompi_info_free (ompi_info_t **info) 170 { 171 (*info)->i_freed = true; 172 OBJ_RELEASE(*info); 173 *info = MPI_INFO_NULL; 174 return MPI_SUCCESS; 175 } 176 from: https://svn.open-mpi.org/source/xref/ompi-trunk/ompi/info/info.c
Anyway both solutions would work, but until now I couldn't find any real disadvantage...
Cheers,
Daniel
Am 11.05.2012 um 21:18 schrieb Peter Tröger:
The main argument against NULLing the pointer is really just principle of least astonishment -- routines in C don't typically do that, especially not in OS libraries. Because there's such a large body of function calls that don't do it, throwing in some that do, can make your code confusing. Did you forget to NULL that pointer, or was it done in the library? NULLing the pointer is more practical, but only if everyone does it that way. I would vote to conform to what the majority of libraries already do.
Completely agreed - there is nothing like this in POSIX, which was the golden standard at least for DRMAAv1.
Best regards, Peter.
Daniel
On 5/11/12 7:51 AM, Daniel Gruber wrote:
Am 11.05.2012 um 16:27 schrieb Klauck, Stefan:
Hi Daniel
On May 11, 2012, at 3:17 PM, Daniel Gruber wrote:
Hi Stefan,
Am 11.05.2012 um 14:49 schrieb Klauck, Stefan:
> Hello, > > I put my comments below. > > Best regards > Stefan > > > On May 9, 2012, at 4:47 PM, Daniel Gruber wrote: > > For the current public comment period I've following issues: > > 295: drmaa2_list_get(.., int pos) -> pos should be const, the > implementation does not need to change it > - 297 int pos -> same > > The 'const' is not really necessary in the header file since the > implementation file can add the const, too. > > The following example is legal c code: > > //foo.h > void foo(int bar); > > //foo.c > void foo(const int bar); > But you will loose the advantage of pushing a const into the function. Do you mean that you do not force the implementation to use a const? Now I'm lost :) Yes, I want to have the implementations that they are forced to use the const. Hence I wan't to have this in the spec like the other consts there. This would allow DRMAA application implementers to pass a const. Having const in the innermost functions is always a good idea in order to avoid bad casts.
You could also avoid "bar" in the header (like void foo(int)), but this makes code hard to maintain. Since we have already "const" in the spec, in my eyes it would look in my eyes more consistent.
I agree. My point was that it is not really necessary to do not allow the implementation to change the 'pos' value since the value is not returned anyway. Nevertheless, the implementation could add the 'const' for optimization… You're right, now we can force them to do so ;)
> > - 549 char **substate -> the user has to free it with > "drmaa2_string_free()". Since it is just a string the user might > "forget" this and use > a "normal" free. > > In the current mock implementation > https://github.com/troeger/drmaav2-mock/blob/master/drmaa2.c > drmaa2_string_free() is a normal free(), so that this would be ok > (except for the NULL'ing). Indeed. Here it wouldn't be a problem.
> Hence I would recommend to have a drmaa2_string type because all > other data (which has to be freed) is drmaa2 typed. > > > Sounds nevertheless reasonable. > > > General question: I would like to have the free functions to NULL > the freed pointers (in order to minimize the risk with > dangling pointers). Hence I would change *all* free methods to > accept ** arguments. Wouldn't this be reasonable? > Why should the user always do the NULL'ing itself? He might forget > or be just lazy and running later in problems, which > we could avoid easily. > > Example: > drmaa2_list_free(&list); > /* some code later, I want to do something with list again, but not > sure if it was freed already */ > ... > if (list == NULL) { > /* oh it was freed already */ > } else { > /* not feed */ > } > > > I had no opinion. Hence I googled the problem. > There are many conversations concerning this topic especially on > stackoverflow > e.g. > http://stackoverflow.com/questions/1879550/should-one-really-set-pointers-to... > . > > Since the "NULL'ing" is controversial, I suggest to do not set the > pointer to NULL within the implementation. > Applications can still implement macros or wrapper functions (for > NULL'ing) to write portable code, in case they want to set freed > pointers to NULL and rely on that. Can't see any valid argument against, sorry could read the full article in all details. I guess we've to vote :) You lose the address of the freed pointer. (That's why I would let the application developer to decide.)
example:
drmaa2_list list_reference = list; drmaa2_list_free(&list); if (list == list_reference) // false Uhh, wasn't aware of this case (sorry, my mistake I wanted to say "couldn't read the full article in all details)...). Is such a reference to an pointer we've really needed in DRMAA apps? I'm wondering what would s.o. could do with this reference pointing to some (most likely) bad memory location. IMHO using a pointer after freeing is nothing s.o. should do by choice. Hence immediately after the list_free() the reference (if it is really needed) should be NULLed as well.
Anybody who really wants to do that? Roger? Andre?
Cheers,
Daniel
Even though list and list_refernce are invalid pointers, it is not really intuitive that the comparison evaluates to 'false'.
In addition "if (list_reference == NULL)" is still 'false'
Cheers Stefan
Cheers,
Daniel
> Thanks, > > Daniel > > > Am 17.04.2012 um 11:16 schrieb Peter Tröger: > > Dear all, > > the DRMAAv2 C binding is now in final draft state. Attached you can > find the annotated and the official version of the specification, > as well as the raw header file. > > Please provide your final comments on the mailing list until *April > 22nd* (this Sunday). In case, we will set up a conf call for last > discussions. Otherwise, the document will enter the OGF document > process on next Monday. > > Thanks and best regards, > Peter. > > > <drmaa-c-finaldraft-annotated.pdf><drmaa-c-finaldraft-official.pdf><drmaa2.h>-- > > drmaa-wg mailing list > drmaa-wg@ogf.org<mailto:drmaa-wg@ogf.org> > https://www.ogf.org/mailman/listinfo/drmaa-wg > > -- drmaa-wg mailing list drmaa-wg@ogf.org https://www.ogf.org/mailman/listinfo/drmaa-wg
-- drmaa-wg mailing list drmaa-wg@ogf.org https://www.ogf.org/mailman/listinfo/drmaa-wg
-- drmaa-wg mailing list drmaa-wg@ogf.org https://www.ogf.org/mailman/listinfo/drmaa-wg
In the MPI case, you have a matching pair of functions - they both take the address to the pointer (ie. &comm ) as the argument: - allocate: MPI_Comm_dup( MPI_COMM_WORLD, &comm ); - free: MPI_Comm_free( &comm ); The difference is that in the DRMAA2 case you get the pointer to the list from the return value of drmaa2_list_create(), and this is the interface used by malloc(). Thus to stay consistent with the most common dynamic memory interface, we should just use the same interface for the free routine. On the other hand, if we were to change the interface of drmaa2_list_create() such that it requires the programmer to pass in the address of a variable to store the return list, then I would agree that we switch to the interface suggested by Daniel Gruber. (I should have reviewed the draft so long ago, but if it wasn't DanT's email, I probably wouldn't have enough motivation to read the whole thing.) Rayson ================================= Open Grid Scheduler / Grid Engine http://gridscheduler.sourceforge.net/ Scalable Grid Engine Support Program http://www.scalablelogic.com/ On Sat, May 12, 2012 at 7:28 AM, Bill Bryce <bbryce@univa.com> wrote:
Good example Daniel. We don't have to do what POSIX does.
Regards
Bill
Sent from my iPad - US cell: 847-612-8966
On 2012-05-12, at 4:42 AM, Daniel Gruber <dgruber@univa.com> wrote:
I wouldn't say that a programmer would be astonished with any solution. The signature implies *unambiguously* what is going to be changed. Of course an OS free() doesn't NULL because it can't. I wouldn't compare the DRMAA obj free with traditional raw free or OS defined functions, which most likely didn't change the last decades.
In popular libraries like OpenMPI (which drmaa app writer most likely use) you have the exact counter-examples:
"int MPI_Comm_free(MPI_Comm *comm)
Description: This operation marks the communicator object for deallocation. The handle is set to MPI_COMM_NULL." (see http://www.open-mpi.org/doc/v1.4/man3/MPI_Comm_free.3.php)
Same is with all other MPI object frees (with exception of a "raw" memory free).
Source of their frees looks like this:
166 /* 167 * Free an info handle and all of its keys and values. 168 */ 169 int ompi_info_free (ompi_info_t **info) 170 { 171 (*info)->i_freed = true; 172 OBJ_RELEASE(*info); 173 *info = MPI_INFO_NULL; 174 return MPI_SUCCESS; 175 } 176
from: https://svn.open-mpi.org/source/xref/ompi-trunk/ompi/info/info.c
Anyway both solutions would work, but until now I couldn't find any real disadvantage...
Cheers,
Daniel
Am 11.05.2012 um 21:18 schrieb Peter Tröger:
The main argument against NULLing the pointer is really just principle
of least astonishment -- routines in C don't typically do that,
especially not in OS libraries. Because there's such a large body of
function calls that don't do it, throwing in some that do, can make your
code confusing. Did you forget to NULL that pointer, or was it done in
the library? NULLing the pointer is more practical, but only if everyone
does it that way. I would vote to conform to what the majority of
libraries already do.
Completely agreed - there is nothing like this in POSIX, which was the golden standard at least for DRMAAv1.
Best regards, Peter.
Daniel
On 5/11/12 7:51 AM, Daniel Gruber wrote:
Am 11.05.2012 um 16:27 schrieb Klauck, Stefan:
Hi Daniel
On May 11, 2012, at 3:17 PM, Daniel Gruber wrote:
Hi Stefan,
Am 11.05.2012 um 14:49 schrieb Klauck, Stefan:
Hello,
I put my comments below.
Best regards
Stefan
On May 9, 2012, at 4:47 PM, Daniel Gruber wrote:
For the current public comment period I've following issues:
295: drmaa2_list_get(.., int pos) -> pos should be const, the
implementation does not need to change it
- 297 int pos -> same
The 'const' is not really necessary in the header file since the
implementation file can add the const, too.
The following example is legal c code:
//foo.h
void foo(int bar);
//foo.c
void foo(const int bar);
But you will loose the advantage of pushing a const into the function.
Do you mean that you do not force the implementation to use a const?
Now I'm lost :) Yes, I want to have the implementations that they are
forced to use the const. Hence I wan't to have this in the spec like the
other consts there. This would allow DRMAA application implementers
to pass a const. Having const in the innermost functions is always a
good idea in order to avoid bad casts.
You could also avoid "bar" in the header (like void foo(int)), but
this makes
code hard to maintain.
Since we have already "const" in the spec, in my
eyes it would look in my eyes more consistent.
I agree. My point was that it is not really necessary to do not allow
the implementation to change the 'pos' value since the value is not
returned anyway.
Nevertheless, the implementation could add the 'const' for optimization…
You're right, now we can force them to do so ;)
- 549 char **substate -> the user has to free it with
"drmaa2_string_free()". Since it is just a string the user might
"forget" this and use
a "normal" free.
In the current mock implementation
https://github.com/troeger/drmaav2-mock/blob/master/drmaa2.c
drmaa2_string_free() is a normal free(), so that this would be ok
(except for the NULL'ing).
Indeed. Here it wouldn't be a problem.
Hence I would recommend to have a drmaa2_string type because all
other data (which has to be freed) is drmaa2 typed.
Sounds nevertheless reasonable.
General question: I would like to have the free functions to NULL
the freed pointers (in order to minimize the risk with
dangling pointers). Hence I would change *all* free methods to
accept ** arguments. Wouldn't this be reasonable?
Why should the user always do the NULL'ing itself? He might forget
or be just lazy and running later in problems, which
we could avoid easily.
Example:
drmaa2_list_free(&list);
/* some code later, I want to do something with list again, but not
sure if it was freed already */
...
if (list == NULL) {
/* oh it was freed already */
} else {
/* not feed */
}
I had no opinion. Hence I googled the problem.
There are many conversations concerning this topic especially on
stackoverflow
e.g.
http://stackoverflow.com/questions/1879550/should-one-really-set-pointers-to...
.
Since the "NULL'ing" is controversial, I suggest to do not set the
pointer to NULL within the implementation.
Applications can still implement macros or wrapper functions (for
NULL'ing) to write portable code, in case they want to set freed
pointers to NULL and rely on that.
Can't see any valid argument against, sorry could read the full
article in all details.
I guess we've to vote :)
You lose the address of the freed pointer. (That's why I would let
the application developer to decide.)
example:
drmaa2_list list_reference = list;
drmaa2_list_free(&list);
if (list == list_reference) // false
Uhh, wasn't aware of this case (sorry, my mistake I wanted to say
"couldn't read the full article in all details)...).
Is such a reference to an pointer we've really needed in DRMAA apps?
I'm wondering what
would s.o. could do with this reference pointing to some (most likely)
bad memory location.
IMHO using a pointer after freeing is nothing s.o. should do by choice.
Hence immediately after the list_free() the reference (if it is really
needed) should be NULLed
as well.
Anybody who really wants to do that? Roger? Andre?
Cheers,
Daniel
Even though list and list_refernce are invalid pointers, it is not
really intuitive that the comparison evaluates to 'false'.
In addition "if (list_reference == NULL)" is still 'false'
Cheers
Stefan
Cheers,
Daniel
Thanks,
Daniel
Am 17.04.2012 um 11:16 schrieb Peter Tröger:
Dear all,
the DRMAAv2 C binding is now in final draft state. Attached you can
find the annotated and the official version of the specification,
as well as the raw header file.
Please provide your final comments on the mailing list until *April
22nd* (this Sunday). In case, we will set up a conf call for last
discussions. Otherwise, the document will enter the OGF document
process on next Monday.
Thanks and best regards,
Peter.
<drmaa-c-finaldraft-annotated.pdf><drmaa-c-finaldraft-official.pdf><drmaa2.h>--
drmaa-wg mailing list
drmaa-wg@ogf.org<mailto:drmaa-wg@ogf.org>
https://www.ogf.org/mailman/listinfo/drmaa-wg
--
drmaa-wg mailing list
drmaa-wg@ogf.org
https://www.ogf.org/mailman/listinfo/drmaa-wg
-- drmaa-wg mailing list drmaa-wg@ogf.org https://www.ogf.org/mailman/listinfo/drmaa-wg
-- drmaa-wg mailing list drmaa-wg@ogf.org https://www.ogf.org/mailman/listinfo/drmaa-wg
-- drmaa-wg mailing list drmaa-wg@ogf.org https://www.ogf.org/mailman/listinfo/drmaa-wg
participants (5)
-
Bill Bryce -
Daniel Gruber -
Daniel Templeton -
Peter Tröger -
Rayson Ho