-
Notifications
You must be signed in to change notification settings - Fork 262
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
Fix documentation for nc_put_att_text
#1510
Conversation
The `nc_put_att_text` function creates an attribute consisting of a single value unlike the other functions which can output more than one value. Update documentation to make this clear. The `len` argument is length of text and not the number of text items.
@@ -83,9 +83,9 @@ be assigned or ::NC_GLOBAL for a global attribute. | |||
\param name Attribute \ref object_name. \ref attribute_conventions may | |||
apply. | |||
|
|||
\param len Number of values provided for the attribute. | |||
\param len Length of `value` not including trailing null. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct. The trailing null may be included for C users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is how i typically used it -- I would pass strlen(text)+1
as the length, but then I grepped the NetCDF source and saw that typically strlen(text)
was passed as the argument. Not sure which is correct...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neither is correct. It's up to the user.
Fortran does not use a NULL, so you might say don't bother writing one.
But then C programs must add a NULL every time they read the attribute. So if writing a bunch of netCDF files that will be read by a bunch of C programs, you might say always include the NULL.
So it is left up to the user. This may be unwise, but that's the way it has been and cannot be changed without breaking lots of code for someone. ;-)
|
||
\param value Pointer to one or more values. | ||
\param value Pointer to text value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well in C there is no "text value". How about:
Pointer to an array of char with one or more elements; the array is the text value of the attribute.
Ed is right. It actually does allow multiple values, namely a sequence of chars. |
Just to share my 2 cents. When I studied NetCDF at the beginning, I was often confused by My understanding of the name As for the argument |
I guess the original documentation is correct considering the attribute as writing 1 or more chars which is similar to the other attribute functions writing 1 or more ints or 1 or more doubles. I am fine with leaving everthing as it is, or if other find it confusing, iterating on this PR... |
I have added an issue for this: #1512 I believe more than just a minor change is called for. I will add some explanation and put up a PR. When we have the C docs all sorted out, I will also add to the Fortran docs. |
OK, I suggest this PR be taken down. I have done an extensive edit of the attribute docs, and have taken into account all comments from the PR. So the new docs should clarify all the issues raised here, as well as some differences between classic and netCDF-4/HDF5 format that were confusing. See #1515 |
Thanks. Will close. |
@gsjaardema thank you for pointing this out! While doing this I noticed some similar problems with the var documentation, so I suppose that's next for me. ;-) |
The
nc_put_att_text
function creates an attribute consisting of a single value unlike the other functions which can output more than one value. Update documentation to make this clear. Thelen
argument is length of text and not the number of text items.