Skip to content

Make IPAddress operators use size_t and always return references#781

Open
spnda wants to merge 1 commit intoPaulStoffregen:masterfrom
spnda:ipaddress_fix_subscript
Open

Make IPAddress operators use size_t and always return references#781
spnda wants to merge 1 commit intoPaulStoffregen:masterfrom
spnda:ipaddress_fix_subscript

Conversation

@spnda
Copy link

@spnda spnda commented Apr 21, 2025

I'm using {fmt} for serial printing in my Teensy 4.1 project, and when working with IPAddress it was not possible to print them trivially, since access to the underlying data was not-accessible as a pointer or reference. This makes it possible to do the following now:

static const auto ip_addr = IPAddress(192, 168, 0, 0); 
fmt::print("{}", fmt::join(&ip_addr[0], &ip_addr[4], "."));

I also think that the IPAddress class should be made constexpr, and perhaps also add a public const alternative to raw_address, but I thought this PR would be trivial enough for now with literally zero impact on current code, while now offering actually "correct" subscript operators that do what one would expect them to do.

While I'm here I wanted to ask what C++ version the cores are compiled with or what the minimum C++ version is, specifically because of the possible compatibility impact of making IPAddress constexpr. I'm also interested in updating some other parts of the codebase to actual C++ or more modern C++, if it makes sense to do so.

@spnda spnda force-pushed the ipaddress_fix_subscript branch from 2522c3b to 993f77d Compare April 21, 2025 18:07
@PaulStoffregen
Copy link
Owner

We use C++17.

The IPAddress API is primarily meant for compatibility with Arduino. Any changes need to be thoroughly tested with Arduino libraries and programs.

@spnda
Copy link
Author

spnda commented Apr 28, 2025

This doesn't affect any previous code, since before you had to expect a copy, and now it returns a reference so existing code will still just take a copy. size_t is int on both Teensy and Arduino as far as I know, and anyway it would be more correct regardless.

I don't think any tests are necessary since this is a trivial change that based on the C++ rules/spec doesn't affect any existing code.

@ssilverman
Copy link
Contributor

I'd be surprised if there wasn't some code or build system somewhere that made an incorrect assumption, such that this change breaks it. I agree that testing with a bunch of existing Arduino libraries and programs is necessary. (I do agree, though, with the principle of making the code "C++ accurate".)

@spnda
Copy link
Author

spnda commented Dec 16, 2025

I'd be surprised if there wasn't some code or build system somewhere that made an incorrect assumption, such that this change breaks it.

But... how? We're switching from returning a prvalue to returning a const lvalue. This is semantically equivalent.

It could only cause a minor problem if somebody has some kind of static assert to check for the return type or takes the address of the function and tries storing it in a variable with an explicitly defined type. Then, and only then, would it not match since the return type is different. However, I find it incredibly unlikely that anybody would do anything like that for such a function, especially in the embedded world. And if such a case ever exists, it gives you an error that is fixable within a couple seconds at most which is, quite frankly, perfectly acceptable. And any other issue from differing function signatures is catchable at compile-time, so this never breaks anything at runtime.

I strongly stand by my position that this change is transparent to 99.99% of users, if not 100%. And if it does, then that developer should get a kick in the butt for writing code that does not adhere to the rules of the programming language they're using. The embedded world needs to stop being scared of change, and write proper code.

@ssilverman
Copy link
Contributor

I don’t disagree with you that it shouldn’t cause a problem. I just think it should be tested with a bunch of Arduino libraries.

@ssilverman
Copy link
Contributor

ssilverman commented Dec 16, 2025

It could even be the case that there’s some rare compiler bug somewhere for some very specific circumstances. I’m just advocating for more testing.

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