tasklwIP - A Lightweight TCP/IP stack - Tasks: task #7410, Don't include internal headers in...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

task #7410: Don't include internal headers in api header files

Submitter:  Simon Goldschmidt <goldsimon>
Submitted:  Sun 28 Oct 2007 02:03:05 PM UTC
   
 
Category:  None Should Start On:  Sun 28 Oct 2007 12:00:00 AM UTC
Should be Finished on:  Sun 28 Oct 2007 12:00:00 AM UTC Priority:  4
Status:  Done Privacy:  Public
Assigned to:  goldsimon Percent Complete:  100%
Open/Closed:  Closed Planned Release:  None
Effort:  0.00

Jump to the original submission

Sun 18 Nov 2007 12:17:12 AM UTC, comment #15: 

ok for me

Frédéric Bernon <fbernon>
Group Member
Sat 17 Nov 2007 06:27:17 PM UTC, comment #14: 

Oh, unless you have other feelings toward ipv6, we can close this as done.

Simon Goldschmidt <goldsimon>
Group administrator
Sat 17 Nov 2007 06:26:21 PM UTC, comment #13: 

The checksum routines were defined in inet.h which was in include/ipv4/lwip. They are still defined in include/ipv6/lwip/inet.h, I didn't make any changes to ipv6...

That's because I don't understand how ipv6 can work at all at the moment: in inet6.c, there are only the checksum functions but not the inet-functions (inet_addr, inet_aton, ...)!

And honestly, before I change it to something that doesn't work either, I have decided to leave it like it is (only I moved inet6.c into the ipv6 folder).

Simon Goldschmidt <goldsimon>
Group administrator
Sat 17 Nov 2007 12:58:14 PM UTC, comment #12: 

About last change, it's fine, but, why inet_chksum.h is in include/ipv4/lwip, and not in include/lwip ? Isn't it shared with IPv6 folder ?

Frédéric Bernon <fbernon>
Group Member
Fri 16 Nov 2007 08:03:00 AM UTC, comment #11: 

Since there is already "lot" of new files in 1.3.0 (netbuf, autoip, igmp...) I think it's good to do it now.

Frédéric Bernon <fbernon>
Group Member
Fri 16 Nov 2007 07:18:18 AM UTC, comment #10: 

We'll have to change makefiles/projects anyway because I'm about to create a new file inet_chksum.c and move the checksum functions there.

Simon Goldschmidt <goldsimon>
Group administrator
Thu 15 Nov 2007 10:48:00 PM UTC, comment #9: 

fine for me, even if we need to change makefiles/projects, it's a minor change, but it's cleaner.

Frédéric Bernon <fbernon>
Group Member
Thu 15 Nov 2007 09:13:30 PM UTC, comment #8: 

What about moving inet.c to ipv4 and inet6.c to ipv6/inet.c??

Simon Goldschmidt <goldsimon>
Group administrator
Mon 12 Nov 2007 10:45:43 PM UTC, comment #7: 

I think it's should be good to split now inet functions and checksim functions before release 1.3.0.

Frédéric Bernon <fbernon>
Group Member
Tue 06 Nov 2007 08:34:03 PM UTC, comment #6: 

Checked in (without changing tcp.h).

Another question (like posted on irc already): Wouldn't it make sense to split inet.h into 'real' inet.h-like functions and checksum-functions? As it is now, sockets.h includes pbuf.h just because inet.h includes the checksum functions (when sockets.h only needs inet_addr() and htonl()).

Simon Goldschmidt <goldsimon>
Group administrator
Tue 06 Nov 2007 08:13:22 PM UTC, comment #5: 

Good for me, as long we avoid to include api.h in tcp.h (IRC chat conclusion).

Frédéric Bernon <fbernon>
Group Member
Tue 06 Nov 2007 07:54:35 PM UTC, comment #4: 

The second patch is fine by me.

Jonathan Larmour <jifl>
Group Member
Tue 06 Nov 2007 07:46:32 PM UTC, comment #3: 

OK, that doesn't sound too excited...

> After all, users can always include api_internal.h and directly
> access to netconn fields


In C++, users can also #define private public before including a class header and then freely access private fields and still people regard this as being protected...

What I want to do is some 'information hiding' where it's not needed. I made a second patch that forward-defines the needed internal structs (mainly the PCB structs).

> I don't think it's good to increase code size just to separate API headers


This second patch definitvely doesn't increase code size, BTW.

(file #14332)

Simon Goldschmidt <goldsimon>
Group administrator
Sun 28 Oct 2007 09:57:35 PM UTC, comment #2: 

If internal lwip structs don't cause any build problems, I'm not sure to understand what is the real advantage (I suppose you want to be able to do some "internal" changes in lwIP, without talking about any "api breakage"). After all, users can always include api_internal.h and directly access to netconn fields. So, it's not really more "protected". If we want to avoid that an user directly access to netconn or netbuf fields , perhaps we could just add some comments in these structs? Like "don't do any direct access to these fields, they can be change in future release, use the API functions for that", or simply "INTERNAL FIELD, DON'T ACCESS IT". In all cases, if most of developers are agree to do that, I would prefer one file per struct/api/file, like "netconn_structs.h" and "netbuf_structs.h" ("structs" and not "struct", because we could declare some others subtypes), because netbuf should be used without netconn.



Frédéric Bernon <fbernon>
Group Member
Sun 28 Oct 2007 02:53:43 PM UTC, comment #1: 

I don't think it's good to increase code size just to separate API headers. Are we saying it's ok to use the internal macros or not? If we're saying people "should" use the functions, not the macros.... except if you want smaller code, then use the internal API... then that doesn't seem like a clean API.

I don't think trying to prevent users seeing internals is worth it - if they want to see the internals they can, so I don't see what is gained.

Jonathan Larmour <jifl>
Group Member
Sun 28 Oct 2007 02:03:05 PM UTC, original submission:  

Picking up the idea from task #6683, I'd like to remove the need to include internal header files in api.h and netbuf.h (sockets.h is already OK).

The reason for this is to prevent the user from seeing the internals when programming applications.

The main idea of my patch is to creat a new file (api_internal.h or whatever you want it to be named), which holds the definitions of the structs netconn and netbuf and including this file in the lwIP internal files but not in the actual (user-)application files.


The downside would be that functions we converted to macros (after 1.2.0 mostly) have to be converted back to functions. But leaving the defines in api_internal.h, users who really want the speedup can just decide to include api_internal.h and use the macros instead.

Patch & api_internal.h is attached.

Simon Goldschmidt <goldsimon>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #14332:  api_h_cleanup.patch added by goldsimon (5KiB - text/x-patch)
file #14234:  api_internal.patch added by goldsimon (7KiB - text/x-patch)
file #14235:  api_internal.h added by goldsimon (2KiB - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by fbernon (Posted a comment)
  • -email is unavailable- added by jifl (Posted a comment)
  • -email is unavailable- added by goldsimon (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2007-11-21 goldsimon StatusNone Done
        Percent Complete0% 100%
        Assigned toNone goldsimon
        Open/ClosedOpen Closed
    2007-11-06 goldsimon Attached File- Added api_h_cleanup.patch, #14332
    2007-10-28 goldsimon Attached File- Added api_internal.patch, #14234
        Attached File- Added api_internal.h, #14235

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code