Bug 2907

Michele, please address these comments:

1) the class Doxygen is not descriptive enough (describe the behavior about how it retries random positions corresponding to independent random variables on X, Y, and Z coordinates, for MaxNumAttempts before aborting)

2) the code doesn't conform to ns-3 coding style for indentation and spaces before parentheses (consider to run the check-style.py script in the utils directory, which calls uncrustify, to produce compliant style)

3) MaxNumAttempts could be better named to MaxAttempts (this would make the naming more consistent with the rest of ns-3)

4) avoid NS_ASSERT and use instead NS_ABORT_MSG_IF so that you can catch errors in non-debug builds

Created attachment 3090 [details]
Proposed patch - V2

Hi Tom,
thanks for your feedback.
I addressed the four comments, please let me know if the documentation is descriptive enough.
Uncrustify found some issues with spaces also in the other classes of the files I am patching, I hope it's fine to leave them in the patch.
Best,
Michele

(In reply to Michele Polese from comment #2)
> Created attachment 3090 [details]
> Proposed patch - V2
> 
> Hi Tom,
> thanks for your feedback.
> I addressed the four comments, please let me know if the documentation is
> descriptive enough.

Yes, but please add also the '\ingroup buildings' and '\brief' components, such as here:

/**
 * \ingroup buildings
 * \brief mobility buildings information (to be used by mobility models)
 *
 * This model implements the managment of scenarios where users might be
 * either indoor (e.g., houses, offices, etc.) and outdoor.
 *
 */


> Uncrustify found some issues with spaces also in the other classes of the
> files I am patching, I hope it's fine to leave them in the patch.

Can you please split your patch from any other whitespace changes, either discarding the unrelated whitespace changes or putting them into a separate 'fix-whitespace.patch'?

We made a maintenance decision a while back to avoid continually tweaking the small whitespace issues (which do accumulate over time) because it tends to breaks out of tree patches, and because the output of uncrustify varies across its versions.  I tend to use 'check-style.py' for any new file that goes into the repository, and then I use it as a guide for any new patches made to existing files but I avoid making unrelated whitespace changes in other portions of the code that are bundled with a new feature patch.

Created attachment 3091 [details]
Proposed patch - V3

Hi Tom,
it makes sense.
This is a patch for the OutdoorPositionAllocator class only.
If you want I can prepare a patch with the \ingroup and \brief information for the other classes in building-position-allocator.h.
Best,
Michele

(In reply to Michele Polese from comment #4)
> Created attachment 3091 [details]
> Proposed patch - V3
> 
> Hi Tom,
> it makes sense.
> This is a patch for the OutdoorPositionAllocator class only.

Thanks for making updates; I think it can be merged now.

> If you want I can prepare a patch with the \ingroup and \brief information
> for the other classes in building-position-allocator.h.

Sure, if you don't mind making other improvements in a separate patch, we'll take them.

(In reply to Tom Henderson from comment #5)
> (In reply to Michele Polese from comment #4)
> > Created attachment 3091 [details]
> > Proposed patch - V3
> > 
> > Hi Tom,
> > it makes sense.
> > This is a patch for the OutdoorPositionAllocator class only.
> 
> Thanks for making updates; I think it can be merged now.
Thanks, will you take care of this?
> 
> > If you want I can prepare a patch with the \ingroup and \brief information
> > for the other classes in building-position-allocator.h.
> 
> Sure, if you don't mind making other improvements in a separate patch, we'll
> take them.
I will prepare them in a few days.

> Thanks, will you take care of this?

Yes, I plan to merge early next week if no other comments.

pushed in 13493:d974ae28f427