Documentation updates and misc. stuff (Was: [Gnustep-cvs] Commit Update)

[Alexander Malmberg]

First, big thanks to Adrian Robert for all this documentation work! Very
nice! (I assume the -base bits in the first commit are yours, too,
despite the missing ChangeLog entry. ;)

Anyway, some comments while looking through the changes:


A big comment on something I noticed about halfway through the patch:

> +Nonetheless, the recommendation of this author is that you put the comments
> +in the header,

Regardless of opinions about this, it's very important that everything
is documented only once, and this doesn't seem to be the case with your
patch. I, too, prefer to have the documentation in the headers, but the
fact that we only have one copy of each bit of documentation is much
more important than its location. This should be fixed as soon as
possible (before the two copies diverge and there's time-consuming
merging to do).

This also applies if there are several implementations eg. due to
#ifdef:s; there must still only be one copy of the documentation.


[snip]
> Index: core/gui/Documentation/GuiUser/DefaultsSummary.gsdoc
[...]
>        <heading>Defaults Summary</heading>
>        <p>
> -        This document contains a summary of available user default
> -        values that one can set to control the operation of the GNUstep
> -        libraries.
> +        This document contains a summary of available user default values 
> that
> +        one can set to control the operation of the GNUstep libraries.  To 
> set
[snip instructions for setting defaults]

Although it's nice to have some basic instructions, and maybe a simple
example here, wouldn't it be better if there was proper documentation of
the defaults tool and (end-)user visible parts of the defaults system in
base, referenced here?

OTOH, it might be more appropriate to collect all end-user documentation
in -gui. Thoughts?

[snip]
>        <p>
> -      Below is a list of defaults used to control the gnustep-gui library.
> +      Below is a list of <code>NSGlobalDomain</code> defaults used to
> +      control the gnustep-gui library.

You don't have to set these defaults in NSGlobalDomain. The value in
NSGlobalDomain applies to all apps "by default", but you can override
for a particular app by setting a value in just that app's domain.

[...]
> +         <term>NSShowNonLocalizedStrings</term>

This is really a -base default, and I don't think it qualifies as a
default for end-users, so I think this should be documented in -base.

[snip]
> Index: core/base/Headers/Foundation/NSAutoreleasePool.h
[...]
> -/* Each thread has its own copy of these variables.
> -   A pointer to this structure is an ivar of NSThread. */
> +/**
> + * Each thread has its own copy of these variables.
[...]
>  typedef struct autorelease_thread_vars
[...]
> -/* Each pool holds its objects-to-be-released in a linked-list of 
> -   these structures. */
> +/**
> + *  Each pool holds its objects-to-be-released in a linked-list of 
> +    these structures.
[...]
>  typedef struct autorelease_array_list

These structures are, afaict, private internal structures in -base.
Won't the /** comment header cause them to be added to the public
documentation? Thus, the original comments seemed more appropriate.

[snip]
> Index: core/base/Headers/Foundation/NSBundle.h
[...]
>  /**
> -   Returns the value for the key found in the strings file tableName,
> -   or Localizable.strings if tableName is nil.
> +   Returns the value for the key found in the strings file tableName, or
> +   Localizable.strings if tableName is nil.  Behavior depends on whether the
> +   user default <code>NSShowNonLocalizedStrings</code> is set.  If set, the
> +   value of the key will be returned as an uppercase string rather than any
> +   localized equivalent found.  This can be useful during development to
> +   check where a given string in the UI is "coming from".
>   */

I'd start a new paragraph before "Behavior depends ..." since that's a
special case and not part of the normal operation of the method.

[...]
> -/** Warning - the following should never be used.  */
> +/** Warning - do not use this.  */
>  #define GSLocalizedString(key, comment) \
>    [[NSBundle gnustepBundle] localizedStringForKey:(key) value:@"" table:nil]
> +/** Warning - do not use this.  */
>  #define GSLocalizedStringFromTable(key, tbl, comment) \
>    [[NSBundle gnustepBundle] localizedStringForKey:(key) value:@"" 
> table:(tbl)]

These methods have, for whatever reason, been deprecated since 2002, and
I can't find any uses of them (core/, {usr,dev}-apps/, dev-libs, other
apps). Is there any reason not to remove them now?

[snip]
> Index: core/base/Headers/Foundation/NSConnection.h
[...]
> +/* PENDING: This is not referenced elsewhere in Base.  Is it still needed? */
> +/** Present for backwards compatibility.  Do not use. */
>  GS_EXPORT NSString *ConnectionBecameInvalidNotification;

Looks like it should be removed. There's no definition, so even if
someone is using it, they're broken already.

[snip]
> Index: core/base/Headers/Foundation/NSDecimal.h
[...]
> +/** Returns whether decimal represents an invalid number. */
>  static inline BOOL
>  NSDecimalIsNotANumber(const NSDecimal *decimal)

A "not a number" (aka. a NaN) is a special kind of number, so it's
misleading to say that it's an invalid number. Better to just say
"Returns whether decimal represents a "not a number"." or something like
that.

[...]
> +/** Copies value of decimal number to preallocated destination. */
>  GS_EXPORT void
>  NSDecimalCopy(NSDecimal *destination, const NSDecimal *source);
>  
> +/** Tries to reduce memory used to store number internally. */
>  GS_EXPORT void
>  NSDecimalCompact(NSDecimal *number);

The wording here is very terse.

[...]
> +/**
> + *  Sets the exponents of n1 and n2 to equal one another, adjusting mantissas

s/to equal/equal to/

> + *  as necessary to preserve values.  This makes certain operations quicker.
> + */
>  GS_EXPORT NSCalculationError
>  NSDecimalNormalize(NSDecimal *n1, NSDecimal *n2, NSRoundingMode mode);
[...]
> +/**
> + *  Multiplies n by 10^power and returns result to 38-digit precision.  See 
> the
> + *  [(NSDecimalNumberBehaviors)] protocol for a description of mode and the
> + *  return value.  The result should be preallocated but can be the same as
> + *  n or power.

s/or power//

> + */
>  GS_EXPORT NSCalculationError
>  NSDecimalMultiplyByPowerOf10(NSDecimal *result, const NSDecimal *n, short 
> power, NSRoundingMode mode);
[snip]
> Index: core/base/Headers/Foundation/NSDistributedNotificationCenter.h
[...]
> +/**
> + *  Enumeration of possible values for specifying how
> + *  [NSDistributedNotificationCenter] deals with notifications when the
> + *  process to which the notification should be delivered is suspended:
> + <example>

A deflist seems more appropriate here. See the documentation for
-addObserver:selector:name:object:suspensionBehavior: (which also has
better descriptions of the values, so either those or the docs here
should reference the other to avoid duplicating it).

> + {
> +  NSNotificationSuspensionBehaviorDrop,       // drop the notification
> +  NSNotificationSuspensionBehaviorCoalesce,   // drop all for this process 
> but the latest-sent notification
> +  NSNotificationSuspensionBehaviorHold,       // queue all notifications for 
> this process until it is resumed
> +  NSNotificationSuspensionBehaviorDeliverImmediately  // resume the process 
> and deliver
> +}
> + </example>
> + */
[snip]
> Index: core/base/Headers/Foundation/NSException.h
[...]
>  /* Exception handler definitions */
> +
> +/**
> + * The actual structure for an NSHandler.  You shouldn't need to worry about 
> it.
> + */
>  typedef struct _NSHandler

I think this is private, so it shouldn't appear in the public docs.

[...]
> +/**
> + *  This is the exception handler called when an exception is generated and

Not really. This is the _type_ of the function that's called when an
exception is raised but not caught. The documentation is good, but it
seems odd to put it on the type. It would be better to put it on the
set/get functions (though that might not be possible while they are
macros, which might be an argument for replacing them with static inline
functions).

> + *  not caught by the programmer (by enclosing in <code>NS_DURING</code> and
> + *  <code>NS_HANDLER</code>...<code>NS_ENDHANDLER</code>).  It prints an 
> error

s/It/The default uncaught exception handler/

> + *  message and exits the program.  You can change this behavior by calling
> + *  <code>NSSetUncaughtExceptionHandler(fn_ptr)</code>.
> + */
>  typedef void NSUncaughtExceptionHandler(NSException *exception);
[...]
> -/* private support routines.  Do not call directly. */
> +/** Private support routine.  Do not call directly. */
>  GS_EXPORT void _NSAddHandler( NSHandler *handler );
> +/** Private support routine.  Do not call directly. */
>  GS_EXPORT void _NSRemoveHandler( NSHandler *handler );

Private, shouldn't be in the public docs.

[snip]
> Index: core/base/Headers/Foundation/NSFileHandle.h
> +/**
> + * Dictionary key for [NSFileHandle] notifications used to mark the
> + * [NSFileHandle] receiving data.
> + */

This is wrong, afaict. NSFileHandleNotificationDataItem gives the data
that has been received, not the file handle that has received it.

[snip]
> Index: core/base/Headers/Foundation/NSFileManager.h
[...]
>  /* File Attributes */
> +/** File attribute key in dictionary returned by
> +    [NSFileManager-fileAttributesAtPath:traverseLink:]. */
>  GS_EXPORT NSString* const NSFileAppendOnly;
> +/** File attribute key in dictionary returned by
> +    [NSFileManager-fileAttributesAtPath:traverseLink:]. */
>  GS_EXPORT NSString* const NSFileCreationDate;
[...]
> +/** File system attribute key in dictionary returned by
> +    [NSFileManager-fileAttributesAtPath:traverseLink:]. */
>  GS_EXPORT NSString* const NSFileSystemFreeNodes;

This seems very excessive. Is there no better way to document these?

[snip]
> Index: core/base/Headers/Foundation/NSGeometry.h
[... abbrev]
>  typedef NSPoint *NSPointArray;
>  typedef NSPoint *NSPointPointer;
>  typedef NSSize *NSSizeArray;
>  typedef NSSize *NSSizePointer;
>  typedef NSRect *NSRectArray;
>  typedef NSRect *NSRectPointer;
[...]

These have got to be the most exceedingly pointless typedefs I've ever
seen. Why do we have them? (Similarly for NSRangePointer.)

>  #endif
>  
> -/* Sides of a rectangle. */
> +/** Sides of a rectangle.
> +<example>

This isn't an example. <list>?

> +{
> +  NSMinXEdge,
> +  NSMinYEdge,
> +  NSMaxXEdge,
> +  NSMaxYEdge
> +}
> +</example>
> + */
>  typedef enum _NSRectEdge NSRectEdge;
>  enum _NSRectEdge
[snip]
> Index: core/base/Headers/Foundation/NSNotificationQueue.h
> ===================================================================
> RCS file: 
> /cvsroot/gnustep/gnustep/core/base/Headers/Foundation/NSNotificationQueue.h,v
> retrieving revision 1.1
> retrieving revision 1.2
> diff -u -r1.1 -r1.2
> --- core/base/Headers/Foundation/NSNotificationQueue.h  31 Jul 2003 23:49:29 
> -0000      1.1
> +++ core/base/Headers/Foundation/NSNotificationQueue.h  29 Jul 2004 15:30:47 
> -0000      1.2
> @@ -54,12 +54,34 @@
>   * Posting styles into notification queue
>   */
>  
> +/**
> + *  Enumeration of possible timings for distribution of notifications handed
> + *  to an [NSNotificationQueue]:
> + <example>

Not an example. <deflist>?

[...]
>  } NSPostingStyle;
>  
> +/**
> + * Enumeration of possible ways to combine notifications when dealing with
> + * [NSNotificationQueue]:
> + <example>

<deflist>

> +{
> +  NSNotificationNoCoalescing,       // don't combine
> +  NSNotificationCoalescingOnName,   // combine all registered with same name
> +  NSNotificationCoalescingOnSender  // combine all registered with same 
> object
> +}
> + </example>
[...]
> +/**
> + *  Structure used internally by [NSNotificationQueue].
> + */
>  struct _NSNotificationQueueList;

Private, so it shouldn't be in the public docs.

[snip]
> Index: core/base/Headers/Foundation/NSObject.h
[...]
> -/* Global lock to be used by classes when operating on any global
> -   data that invoke other methods which also access global; thus,
> -   creating the potential for deadlock. */
> +/** Global lock to be used by classes when operating on any global
> +    data that invoke other methods which also access global; thus,
> +    creating the potential for deadlock. */
>  GS_EXPORT NSRecursiveLock *gnustep_global_lock;

Private (at least arguably).

[snip]
> Index: core/base/Headers/Foundation/NSPathUtilities.h
[...]
> +/**
> + * Enumeration of possible requested directory type specifiers for
> + * NSSearchPathForDirectoriesInDomains() function.  These correspond to the
> + * subdirectories that may be found under, e.g., $GNUSTEP_SYSTEM_ROOT, such
> + * as "Library" and "Applications".
> + <example>

<deflist> or <list>. There are a whole bunch of these, so I won't
comment on the rest of them. You should just check all uses of <example>
and make sure that the appropriate tag is used instead.

[snip]
> Index: core/base/Headers/Foundation/NSRange.h
[...]
> +/** Convenience method for raising an NSRangeException. */
>  GS_EXPORT void _NSRangeExceptionRaise (void);

Private.

[...]
> +/** Parses range from string of form {location=a, length=b}, otherwise 
> returns

s/otherwise//

> +    range with 0 location and length if this fails. */
>  GS_EXPORT NSRange NSRangeFromString(NSString *aString);
[...]
> -/*
> +/**
>   * To be used inside a method for making sure that a range does not specify
> - * anything outsize the size of an array/string.
> + * anything outside the size of an array/string.  Raises exception if range
> + * extends beyond [0,size).
>   */
>  #define GS_RANGE_CHECK(RANGE, SIZE) \

Is this really intended to be public?

> +/** Checks whether INDEX is strictly less than OVER (within C array space). 
> */
>  #define CHECK_INDEX_RANGE_ERROR(INDEX, OVER) \

Idem.

> Index: core/base/Headers/Foundation/NSRunLoop.h
[...]
> -/* Mode strings. */
> +/**
> + * Run loop mode used to deal with input sources other than NSConnections or
> + * dialog windows.  Most commonly used. Defined in
> + * <code>Foundation/NSRunLoop.h</code>.
> + */
>  GS_EXPORT NSString * const NSDefaultRunLoopMode;

Why explicitly say where it's defined?

[...]
> +/**
> + * GNUstep extension: enumeration of event types that an [NSRunLoop] watcher

s/GNUstep extension: e/E/

If it's really wanted, I'd mention it with a note at the end of the
documentation.

> Index: core/base/Headers/Foundation/NSString.h
[...]
> -/* For internal use with NeXT runtime;
> -   needed, until Apple Radar 2870817 is fixed. */
> +/** For internal use with NeXT runtime;
> +    needed, until Apple Radar 2870817 is fixed. */
>  extern struct objc_class _NSConstantStringClassReference;

Definitely private. :)

> Index: core/base/Headers/Foundation/NSThread.h
[...]
> +/**
> + *  Notification posted the first time a new [NSThread] is created or a
> + *  separate thread from another library is registered in an application.

This is slightly misleading. The notification is (for things to work
properly; iirc, there are issues with the code) posted _before_ a second
thread is created or allowed to use GNUstep, but there's no guarantee
that a second thread will be created just because the notification is
posted.

> +/**
> + *  Notification posted (only in GNUstep) whenever a new thread is started 
> up.

s/(only in GNUstep) //

>  GS_EXPORT NSString     *NSThreadDidStartNotification;
[snip]
> Index: core/base/Headers/Foundation/NSURLHandle.h
[...]
> +/**
> + * Key for passing to [NSURLHandle]'s <code>propertyForKey..</code> methods 
> to
> + * obtain status code.
> + */
>  GS_EXPORT NSString * const NSHTTPPropertyStatusCodeKey;
> +
> +/**
> + * Key for passing to [NSURLHandle]'s <code>propertyForKey..</code> methods 
> to
> + * obtain status reason.
> + */
>  GS_EXPORT NSString * const NSHTTPPropertyStatusReasonKey;
[...]
> +/**
> + * Key for passing to [NSURLHandle]'s <code>propertyForKey..</code> methods 
> to
> + * obtain proxy port.
> + */
>  GS_EXPORT NSString * const GSHTTPPropertyProxyPortKey;

Again, this is very ugly in the source. If we don't have a way of
handling this better, we should add one.

[snip]
> Index: core/base/Headers/Foundation/NSUndoManager.h
[...]
> +/**
> + *  Notification posted whenever [NSUndoManager] opens or closes an undo 
> group.
> + *  The undo manager itself is posted, with no <em>userInfo</em> dictionary.

Do you mean that the undo manager is the notification's object?

[snip]
> Index: core/base/Headers/Foundation/NSUserDefaults.h
[...]
> +/**
> + *  Array of arrays, first member of specifying a time, next members one or

s/next/following/
Would still a bit oddly worded, though.

[...]
> +/** Key for locale dictionary: one or more strings designating the next
> +    day, such as "day after tomorrow". */

The "day after tomorrow" isn't the  "next day". :)

>  GS_EXPORT NSString* const NSNextNextDayDesignations;
[...]
> -   OpenStep spec currently is either complete nor consitent. Therefor
> -   we had to take several implementation decisions which make vary in
> +   OpenStep spec currently is either complete nor consistent. Therefore

s/either/neither/

> +   we had to take several implementation decisions which may vary in

s/take/make/

>     different OpenStep implementations.

s/different/other/

[snip]
> Index: core/base/Headers/Foundation/NSValue.h
[...]
> +/**
>   * Cache info for internal use by NSNumber concrete subclasses.
[...]
>  } GSNumberInfo;

Private.

> +/** Internal method for caching. */
>  GSNumberInfo   *GSNumberInfoFromObject(NSNumber *o);

Private.

> +/**
> + * Internal method: get cached values for integers in the range
> + * - GS_SMALL to + GS_SMALL
>   */
>  unsigned       GSSmallHash(int n);

Private.

[snip]
> Index: core/base/Headers/Foundation/NSZone.h
[...]
> +/**
> + * Primary structure representing an <code>NSZone</code>.  Technically it
> + * consists of a set of function pointers for zone upkeep functions plus some
> + * other things-

s/-/:/ ? However, since this is documented (with identical
documentation) at the real struct definition, the first copy should be
removed.

[...]
> +/**
> + * Primary structure representing an <code>NSZone</code>.  Technically it
> + * consists of a set of function pointers for zone upkeep functions plus some
> + * other things-

The first copy of this documentation block should be removed.

[...]

The rest of NSZone.h is a bit messy. It seems that everything is
documented twice, which is bad. There should only be one copy of each
documentation block. If necessary, we could move forward declarations of
the functions above the #if GS_WITH_GC and put the documentation there.
Thus, I haven't looked closer at the changes there or in NSZone.m.

- Alexander Malmberg