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

Added _t to a couple of the NVS typedefs (IDFGH-891) #3239

Closed
wants to merge 0 commits into from

Conversation

badcf00d
Copy link
Contributor

@badcf00d badcf00d commented Apr 1, 2019

Just went through and added _t to some typedefs in nvs.h to match the convention. This mainly helps with syntax highlighting, vscode recognises _t as a typedef and highlights it

@CLAassistant
Copy link

CLAassistant commented Apr 1, 2019

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot changed the title Added _t to a couple of the NVS typedefs Added _t to a couple of the NVS typedefs (IDFGH-891) Apr 1, 2019
Copy link
Contributor

@projectgus projectgus left a comment

Choose a reason for hiding this comment

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

Thanks for thinking of us and sending this PR.

I'm going to ask @igrr to weigh in here, because I think he originally chose this name.

There are some complicating factors here:

  • Technically, typename_t is reserved by POSIX for use by POSIX types only. Clearly, a large percentage of the world's C code violates this rule and we've violated this rule in many other parts of ESP-IDF.
  • Given that we've violated the rule elsewhere, it may be better that we're consistent and violate it here as well. Although I'm not sure about this, especially if we wanted to "do the right thing" in the future.

@@ -26,7 +26,7 @@ extern "C" {
/**
* Opaque pointer type representing non-volatile storage handle
*/
typedef uint32_t nvs_handle;
typedef uint32_t nvs_handle_t;
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably don't want to break all existing projects' code which uses nvs_handle, so at minimum we would want another typedef here to maintain compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep that's a good point, added back the original typedefs I replaced

@projectgus projectgus requested a review from igrr April 2, 2019 06:55
Copy link
Member

@igrr igrr left a comment

Choose a reason for hiding this comment

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

There seems to be more code in IDF which uses _t suffix in typedefs than the code which doesn't, so we can probably stick to the "wrong" option. As long as the types are namespaced with an appropriate prefix, the risk of conflicting with a POSIX type should be low.

Agree that a typedef for compatibility with the existing code is needed.

@badcf00d
Copy link
Contributor Author

badcf00d commented Apr 2, 2019

Ah ok I didn't realise the _t was originally a POSIX thing, I had just seen it used as the convention when declaring typedefs, but as @igrr says there seems to be more code in the IDF that uses _t than doesn't.

@projectgus
Copy link
Contributor

Thanks for making those changes, @Frosticles . I've cherry-picked this into our internal review & merge queue, with one small additional small change (added a backwards-compatible nvs_openmode to go with nvs_openmode_t).

This PR will be updated when it's merged.

igrr pushed a commit that referenced this pull request May 29, 2019
@badcf00d badcf00d closed this Jul 26, 2019
trombik pushed a commit to trombik/esp-idf that referenced this pull request Aug 9, 2019
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

4 participants