Bug 2900

Summary:	Use forward declaration as often as possible
Product:	ns-3	Reporter:	sebastien.deronne
Component:	general	Assignee:	ns-bugs <ns-bugs>
Status:	CONFIRMED ---
Severity:	enhancement	CC:	pdbarnes, tomh, tommaso.pecorella
Priority:	P3
Version:	unspecified
Hardware:	All
OS:	All

Description sebastien.deronne 2018-03-08 15:54:18 EST

Nat: 
We should include less, we should forward-declare more.

Tom: 
I agree, there still could be more work done on this point.  Several years ago, Daniel Camara worked on using deheader in our codebase (see bugs 1237 and 1832).  We did do some patching based on this but I recall that the compilation time gains were not dramatic.  Perhaps after we get through Robert's latest Windows-related patches, this could be another performance improvement task for someone to revisit.
We could be more insistent on this in our style guide or checklist for submitters and reviewers (e.g. "Has the code been reviewed to remove unnecessary includes and use forward declarations where possible?").


I really think this would be beneficial to reduce compilation time.
I suggest that each maintainer enroll this task for his module (I am personally planning to improve this for the wifi module).

Comment 1 Tommaso Pecorella 2018-03-08 18:34:04 EST

It's not just a matter of compilation time, it's more a re-compilation time thing.
The more we include, the more will be recompiled when a header changes.

Moreover, some strange stuff might happen, like the necessity to include two different headers for IPv6 and only one for IPv4, purely because the IPv4 one have an unnecessary include inside.

I'm a fan of including only the bare minimum. Any effort in removing includes is most welcome, even when it means that we have to *add* something somewhere. 

T.

Comment 2 Peter Barnes 2018-03-10 00:32:32 EST

Another include revision we should move towards:  ns-3 headers *before* system headers.  This will help catch ns-3'isms which hide system definitions.  I know I'm guilty of doing this wrong :)

Comment 3 sebastien.deronne 2018-03-31 04:41:35 EDT

I have a first part done for the wifi module:
https://codereview.appspot.com/345760043

Comment 4 Tom Henderson 2018-04-01 13:34:27 EDT

(In reply to sebastien.deronne from comment #3)
> I have a first part done for the wifi module:
> https://codereview.appspot.com/345760043

I support merging this but please tune the patch (and all such patches that add forward declarations) by making sure that Python bindings can be rescanned and compiled before committing.  Python API scan is not as compatible as g++ is with the aggressive use of forward declaration.

Comment 5 sebastien.deronne 2018-04-01 14:50:39 EDT

Tom, thanks for your feedback, I will rescan python before committing. Is it ok if I check only on a 64 bits machine? Or should I also try on 32 bits? And on macOS?

Comment 6 Tom Henderson 2018-04-01 14:54:32 EDT

(In reply to sebastien.deronne from comment #5)
> Tom, thanks for your feedback, I will rescan python before committing. Is it
> ok if I check only on a 64 bits machine? Or should I also try on 32 bits?
> And on macOS?

64 bits Linux is fine; I can repair 32 bit if needed.   Please check also the downstream mesh and wave modules which may be affected.

Comment 7 Tom Henderson 2018-04-02 11:31:28 EDT

More aggressive use of forward declaration may result in a failure of Python bindings to compile, due to 'invalid use of incomplete type' error.  This error and remedy is described here:

https://www.nsnam.org/wiki/Python_bindings#.22invalid_use_of_incomplete_type.22

Comment 8 sebastien.deronne 2018-04-08 12:41:27 EDT

I will progressively push patches for wifi part, since I am encountering Python binding issues with some of them.