Skip to content

Separate declarations and source #12

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

TrebledJ
Copy link

@TrebledJ TrebledJ commented May 26, 2023

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 added 2 commits May 26, 2023 23:08
follow library assumptions:
> Uses '+' for 62, '/' for 63, '=' for padding
@Densaugeo
Copy link
Owner

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.

Copy link

@v1r4lv0rt3x v1r4lv0rt3x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this refactor and cleanup! The separation of implementation into base64.cpp is a nice improvement, and the Makefile/tests updates make the project more maintainable.

Suggestion:
Consider updating the include guard in base64.hpp to use the more modern and concise:

#pragma once

This helps improve readability and eliminates the chance of mismatched macro names.

Other than that, everything looks good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants