Skip to content

Conversation

@diondokter
Copy link
Contributor

I should have contributed earlier because that would've saved people some work.
Anyways, I worked on this while updating nrfxlib and making a new async wrapper around it.

I think the current strtoul implementation is wrong. It only takes a single string parameter while the real function has more: https://cplusplus.com/reference/cstdlib/strtoul/

My implementation is a port from a C implementation I found (I believe it was from this one: https://github.com/gcc-mirror/gcc/blob/master/libiberty/strtoul.c)

Also I made string precision support for the printf function.
Nrfxlib has a new API for AT commands where it calls printf. I want the users to not have to care about putting in a null byte after all their AT command strings. So I need a way to print up until a given length instead of up until a null byte. See the usage here: https://github.com/diondokter/nrf-modem/blob/c4a01f5062b86771df57fc283a5d4c82f24eec39/src/at.rs#L163-L169

@jonathanpallant
Copy link
Collaborator

As each implementation can come from a different place, the general rule is to put the licence information into each file.

You've removed my copyright and Blue Oak Model Licence statement from strtoul, which is fair as you've also removed the implementation. But you've stated above the implementation is copied from libiberty, which has licence text at the top that you need to follow. Specifically:

 * Redistribution and use in source and binary forms, with or without
 * modification, are permitted provided that the following conditions
 * are met:
 * 1. Redistributions of source code must retain the above copyright
 *    notice, this list of conditions and the following disclaimer.
 * 2. Redistributions in binary form must reproduce the above copyright
 *    notice, this list of conditions and the following disclaimer in the
 *    documentation and/or other materials provided with the distribution.
 * 3. [rescinded 22 July 1999]
 * 4. Neither the name of the University nor the names of its contributors
 *    may be used to endorse or promote products derived from this software
 *    without specific prior written permission.

I am not a lawyer, but doing a source conversion from C to Rust and publishing the result would appear to me to be a copyright violation unless performed within the licence terms offered.

@jonathanpallant
Copy link
Collaborator

The licence also needs to be added to LICENCES.md

@diondokter
Copy link
Contributor Author

I'm also not a lawyer, so I'll just add the license info

@diondokter
Copy link
Contributor Author

Oh well, this is strange. Atoi fails on Linux but not on my Windows...

@diondokter
Copy link
Contributor Author

Should be good now

src/strtoul.rs Outdated
// Rust doesn't support "\f"
const FEED: u8 = 0x0C;
const SPACE_CHARACTERS: [u8; 6] = [b' ', b'\n', b'\t', VERTICAL_TAB, FEED, b'\r'];
SPACE_CHARACTERS
Copy link
Collaborator

Choose a reason for hiding this comment

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

SPACE_CHARACTERS.contains(&argument) would match the other functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, fixed it

@diondokter
Copy link
Contributor Author

Oh, whoops, forgot to fmt

@diondokter
Copy link
Contributor Author

So ehh, do I have to merge this? It's been approved, but I don't see a button for me to click. Or does somebody else need to?

@jonathanpallant
Copy link
Collaborator

I guess I need to?

@jonathanpallant jonathanpallant merged commit 12bb026 into rust-embedded-community:master Dec 8, 2022
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.

2 participants