Bug 167

Summary:	helper API alignment patch
Product:	ns-3	Reporter:	Tom Henderson <tomh>
Component:	helpers	Assignee:	ns-bugs <ns-bugs>
Status:	RESOLVED FIXED
Severity:	normal
Priority:	P3
Version:	pre-release
Hardware:	All
OS:	All
Attachments:	the proposed patch

Description Tom Henderson 2008-04-13 17:43:17 UTC

This proposed patch:
- changes MobilityHelper::Layout to MobilityHelper::Install, and OlsrHelper::Enable to OlsrHelper::Install
  -- basically, trying to bring all helpers to uniformly use "Install()" as the method name for either adding interfaces or aggregating components

- prints out an error in OlsrHelper::Install or InternetStackHelper::Install if the user tries to call it twice on the same node (rather than letting Object::AggregateObject() assert-- this should be more helpful error message

- reverts the change I made in response to bug 162 to allow AddInternetStack() to not fail when it is called twice on the same note (enforces the policy that attempts to aggregate objects twice are programming errors)

Comment 1 Tom Henderson 2008-04-13 17:43:49 UTC

Created attachment 126 [details]
the proposed patch

The proposed patch

Comment 2 Mathieu Lacage 2008-04-13 22:00:14 UTC

The patch looks good. Reviewing it made me think about another the InstallAll methods you have added. Maybe we should consider using the All postfix consistently across this API. For example, CsmaHelper::EnableAscii (void) could be renamed...

Do you want me to report this in another bug report ?

Comment 3 Tom Henderson 2008-04-13 23:31:03 UTC

(In reply to comment #2)
> The patch looks good. Reviewing it made me think about another the InstallAll
> methods you have added. 

I did not add this method (was previously "EnableAll").

> Maybe we should consider using the All postfix
> consistently across this API. For example, CsmaHelper::EnableAscii (void) could
> be renamed...
> 
> Do you want me to report this in another bug report ?
> 

If you mean EnableAllAscii (), yes, that would seem to be bring more alignment with InstallAll().

Comment 4 Mathieu Lacage 2008-04-14 11:21:50 UTC

> > Maybe we should consider using the All postfix
> > consistently across this API. For example, CsmaHelper::EnableAscii (void) could
> > be renamed...
> > 
> > Do you want me to report this in another bug report ?
> > 
> 
> If you mean EnableAllAscii (), yes, that would seem to be bring more alignment
> with InstallAll().

I would say EnableAsciiAll to be consistent :)

Comment 5 Tom Henderson 2008-04-22 23:42:44 UTC

Closed with:
changeset 2995	b72805b3ca69
changeset 2996	a83b94e277d4