[patch #9876] Const-correctness fixes, pointer types for buffers & couple minor tweaks

[yuanjianmin]

URL:
  <https://savannah.nongnu.org/patch/?9876>

                 Summary: Const-correctness fixes, pointer types for buffers &
couple minor tweaks
                 Project: lwIP - A Lightweight TCP/IP stack
            Submitted by: freddie_chopin
            Submitted on: Thu 05 Dec 2019 09:25:38 PM UTC
                Category: PPP
                Priority: 5 - Normal
                  Status: None
                 Privacy: Public
             Assigned to: None
        Originator Email:
             Open/Closed: Open
         Discussion Lock: Any
         Planned Release: None

    _______________________________________________________

Details:

The major part of these patches is changing `u8_t*` to `const void*` for
pppos_input_tcpip(), pppos_input() and pppos_output_cb_fn(). There is no good
reason to use non-const buffers for these functions (and this is possibly
dangerous - why should output callback modify what lwIP gives it or why should
lwIP modify in-place what was given by user?).

While changes to pppos_input_tcpip() and pppos_input() are mostly harmless and
should not affect any application, change to pppos_output_cb_fn() (which I
actually consider the most important) is going to be non-backward compatible
and breaking change in all possible cases. Unfortunately users will now have
to change their prototypes (and maybe also slightly change the
implementation). This is evident from the changes which I had to make in the
example apps.

There is a similar problem for the functions from sio.h - while it may be
debatable whether one should use `u8_t*` or `void*`, functions which do
"writes" should use const pointers in all cases. Please let me know whether
you're open to changing that.

Please compile-test before (hopefully) submitting, as I was probably not able
to build every important combination of options and neither did I try to build
the test apps.



    _______________________________________________________

File Attachments:


-------------------------------------------------------
Date: Thu 05 Dec 2019 09:25:38 PM UTC  Name:
0001-Use-const-void-instead-of-u8_t-in-pppos_input_tcpip.patch  Size: 2KiB  
By: freddie_chopin

<http://savannah.nongnu.org/patch/download.php?file_id=48006>
-------------------------------------------------------
Date: Thu 05 Dec 2019 09:25:38 PM UTC  Name:
0002-Use-const-void-instead-of-u8_t-in-pppos_input.patch  Size: 2KiB   By:
freddie_chopin

<http://savannah.nongnu.org/patch/download.php?file_id=48007>
-------------------------------------------------------
Date: Thu 05 Dec 2019 09:25:38 PM UTC  Name:
0003-Remove-useless-cast-from-pppos_input_sys.patch  Size: 846B   By:
freddie_chopin

<http://savannah.nongnu.org/patch/download.php?file_id=48008>
-------------------------------------------------------
Date: Thu 05 Dec 2019 09:25:38 PM UTC  Name:
0004-Use-const-void-instead-of-u8_t-in-pppos_output_cb_fn.patch  Size: 3KiB  
By: freddie_chopin

<http://savannah.nongnu.org/patch/download.php?file_id=48009>

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9876>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel

[yuanjianmin]

Additional Item Attachment, patch #9876 (project lwip):

File name: 0005-Remove-trailing-whitespaces-in-pppos_example.c.patch Size:2 KB
   
<https://savannah.nongnu.org/file/0005-Remove-trailing-whitespaces-in-pppos_example.c.patch?file_id=48010>

File name: 0006-Remove-useless-cast-from-pppos_output_append-and-ppp.patch
Size:1 KB
   
<https://savannah.nongnu.org/file/0006-Remove-useless-cast-from-pppos_output_append-and-ppp.patch?file_id=48011>



    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9876>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel

[yuanjianmin]

Follow-up Comment #1, patch #9876 (project lwip):

Well… erm… I have mixed feelings about an API change just to change a
pointer type (void* and u8_t* is basically the same thing) and adding some
const hints to compiler. It does not fix any issue, does not add a feature,
does not even prepare to welcome a new feature, in my opinion it is quite
close to being just cosmetic and nitpick.

That would have been great before the 2.x release, now I'm not sure breaking
something that works with something doing exactly the same thing but in a
slightly different and incompatible way is worth it.

Sylvain

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9876>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel

[yuanjianmin]

Follow-up Comment #2, patch #9876 (project lwip):

[comment #1 comment #1:]
> void* and u8_t* is basically the same thing

Unless you are using C++, in which case they are not. When the API requires an
u8_t* and you have a void*, then you have to use `static_cast<u8_t*>()`,
otherwise the compiler produces an error.

> and adding some const hints to compiler. It does not fix any issue, does not
add a feature

Unless you are using C++, in which case removing `const` has to be a separate
step from converting the type and now your calls to lwIP APIs are starting to
look a bit ridiculous:

pppos_input_tcpip(pcb_, static_cast<u8_t*>(const_cast<void*>(buffer)), size);

In my opinion the lack of `const` is a type error in PPP code, therefore I
wouldn't call it "a hint" or "a cosmetic change". The "feature" this adds is
relieving the user from having to cast everything all over the place. Note
that this even allows removing a few casts from within PPP code, which were
required only because PPP used different type (u8_t*) than the rest of lwIP
(void* in payloads)

Final decision is obviously yours (;

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9876>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel

[yuanjianmin]

Follow-up Comment #3, patch #9876 (project lwip):

Hmm, I'm undecided. It's an API change, after all, and a rather cosmetic
one... Maybe we can wait for the next "big" release for such a change.

The rest of the patches might still be valid?

    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9876>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel

[yuanjianmin]

Update of patch #9876 (project lwip):

                  Status:                    None => Done                  
             Assigned to:                    None => gradator              
             Open/Closed:                    Open => Closed                
                 Summary: Const-correctness fixes, pointer types for buffers &
couple minor tweaks => PPP: Const-correctness fixes, pointer types for buffers
& couple minor tweaks

    _______________________________________________________

Follow-up Comment #4:

This is not such a big change after all. I have applied all patches, thank you
Freddie !


    _______________________________________________________

Reply to this item at:

  <https://savannah.nongnu.org/patch/?9876>

_______________________________________________
  Message sent via Savannah
  https://savannah.nongnu.org/


_______________________________________________
lwip-devel mailing list
[hidden email]
https://lists.nongnu.org/mailman/listinfo/lwip-devel