Skip to content
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

components/driver: 'const' all config calls. #519

Merged
merged 1 commit into from Apr 21, 2017

Conversation

buserror
Copy link
Contributor

Some were, some weren't. They all could/should be.

Signed-off-by: Michel Pollet buserror@gmail.com

Some were, some weren't. They all could/should be.

Signed-off-by: Michel Pollet <buserror@gmail.com>
@CLAassistant
Copy link

CLAassistant commented Apr 13, 2017

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@bvernoux bvernoux left a comment

Choose a reason for hiding this comment

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

It will be even better to do const xx_config_t* const for all read only configuration
Example: gpio_config(const gpio_config_t * const pGPIOConfig)
Which means is a constant pointer to a constant value, you cannot change neither where the pointer points nor the value of the pointer.
Also a good practice is to specify for each arguments a comment like /*in*/ /*out*/ or even /*in/out*/ in each API but I'm not sure you have such rules for esp-idf API (and that will requires a refactor of the whole code)

@buserror
Copy link
Contributor Author

@bvernoux I went for consistency with the other ones, as I didn't want to submit a big patch; we can always do it as a second round. I'd like to have that, as you can then also const the config struct you pass, and initializes it with c99 notation instead of using code to do it.

@projectgus
Copy link
Contributor

Thanks Michel, this look good. I've pushed it into our internal review/merge queue.

Regarding const-ifying all arguments (including values), to some extent this is a matter of personal preference. I used to be in the habit of doing it but got talked out of it some years ago. Pointers to const data have the most significance (due to their implications for the calling code, for storage type of the parameter, and the possible optimisations the compiler can apply.)

FWIW, our doxygen includes @param[out] annotations for "out" pointer arguments. It's probably not as consistent as it should be across the entire API, though. :|

Angus

@projectgus projectgus added the Status: Pending blocked by some other factor label Apr 21, 2017
@igrr igrr merged commit 349a77c into espressif:master Apr 21, 2017
igrr pushed a commit that referenced this pull request Apr 21, 2017
components/driver: 'const' all config calls.

Merges PR #519 #519

See merge request !687
@igrr igrr removed the Status: Pending blocked by some other factor label Nov 14, 2017
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.

None yet

5 participants