base64_arduino icon indicating copy to clipboard operation
base64_arduino copied to clipboard

Separate declarations and source

Open TrebledJ opened this issue 1 year ago • 1 comments

This PR introduces a .cpp file to allow the library to be included in multiple places. This is a revival of previous PR attempts: #2, #7. I believe previous PRs have already addressed the need for such a change.

I've ensured the program compiles with make test. Surprisingly, the headers and implementation had different signatures. I've also fixed some issues with a test case violating the assumption that 62 is + and 63 is /.

To address some of your (valid) concerns from previous PRs for posterity.

I have been advised many times that header-only libraries are simpler to build, and that's been my experience.

True. The compiler needs more work creating a separate object file and linking.

Traditionally, people have kept large implementations contained to headers by using inline/static. Problem with this is that this will either 1) increase code size by inlining the function everywhere or 2) cause multiple separate functions (since static keeps symbols local within modules).

Nowadays, header-only files are mostly used with classes (which inlines everything by default) and templates.

To save space, we usually use a source file instead of keeping everything to a header using inline/static. It should compile faster as well, especially when using a build system such as make/cmake, which won't recompile unless changed. Most other Arduino projects I see do this as well: DHT, Adafruit stuff.

One thing that stands out to me: have you seen issues with multiple inclusion of this library, and if so using what compiler? In all the compilers I've used, include guards were enough to prevent that.

Multiple inclusion within the same source isn't the issue here. It's with multiple inclusion in different sources. For example:

// foo.cpp
#include <base64.hpp>
// main.cpp
#include <base64.hpp>
int main() {}

Error:

duplicate symbol 'encode_base64_length(unsigned int)' in:
    /var/folders/9g/h__z_mc12250_nq_k4km9sy80000gn/T/foo-528663.o
    /var/folders/9g/h__z_mc12250_nq_k4km9sy80000gn/T/bar-c1f74c.o
duplicate symbol 'decode_base64_length(unsigned char const*, unsigned int)' in:
    /var/folders/9g/h__z_mc12250_nq_k4km9sy80000gn/T/foo-528663.o
    /var/folders/9g/h__z_mc12250_nq_k4km9sy80000gn/T/bar-c1f74c.o
...
(etc.)
...
ld: 8 duplicate symbols for architecture x86_64

Multiple functions are defined in different sources, and the linker no likey because it doesn't know which symbol to link.

Imagine our pain and struggle when working on a project and being forced to include a file only in a single place. 😩

TrebledJ avatar May 26 '23 15:05 TrebledJ

Thanks for the PR...I'm traveling for the next couple weeks, but I'll be sure to take a look at this when I'm free.

Densaugeo avatar May 29 '23 09:05 Densaugeo