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

Add named shadows support #56

Merged
merged 8 commits into from
Jan 20, 2021
Merged

Conversation

gregbreen
Copy link
Contributor

@gregbreen gregbreen commented Nov 15, 2020

Issue #, if available:
#55

None

Description of changes:

Added support for named shadows. Required for a project I'm working on.

The major design choice was the decision to parameterize (with new shadowName parameter) all of the existing shadow macros, rather than make a new set of macros for named shadows.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@gregbreen gregbreen force-pushed the feature/named-shadows branch 3 times, most recently from 7923fae to c953e13 Compare November 16, 2020 02:57
@gregbreen gregbreen force-pushed the feature/named-shadows branch from bfecd06 to 31deba9 Compare December 12, 2020 13:15
@gregbreen
Copy link
Contributor Author

@sarenameas @leegeth I synchronized the branch with latest main and resolved the merge conflicts. So this is again merge-able. I hope you may be able to look at this soon. I'm happy to address comments over Xmas/NY if you can make comments before Xmas.

source/include/shadow.h Outdated Show resolved Hide resolved
source/include/shadow.h Show resolved Hide resolved
source/include/shadow.h Outdated Show resolved Hide resolved
source/include/shadow.h Outdated Show resolved Hide resolved
source/include/shadow.h Outdated Show resolved Hide resolved
source/include/shadow.h Outdated Show resolved Hide resolved
source/include/shadow.h Outdated Show resolved Hide resolved
source/shadow.c Outdated Show resolved Hide resolved
source/shadow.c Outdated Show resolved Hide resolved
source/include/shadow.h Outdated Show resolved Hide resolved
source/include/shadow.h Outdated Show resolved Hide resolved
@gregbreen gregbreen force-pushed the feature/named-shadows branch from 39e7c1f to 7faa10f Compare January 11, 2021 13:07
@gregbreen
Copy link
Contributor Author

Per some offline conversation, I updated the PR to retain the old API functions and function-like macros for backwards compatibility.

I wasn't keen to essentially duplicate all of those macros, so we could have a second set that each added a shadowName argument. Also, I couldn't really do that anyway because the long macro names were already exceeding 31 characters; longer (new) names contravene MISRA Rule 5.1, requiring external identifiers to be distinct in the first 31 characters.

So instead I've collapsed the API down to pretty much just two function-like macros (one for length computation and one for string assembly), with these accepting the operation as an extra argument (instead of a specialized macro for each operation). Backwardly-compatible macros marked as deprecated.

I spent days thinking about the semantics on this. Not sure it's palatable to others, but I feel confident it's better than before (as well as retaining backwards compatibility). I won't update the main device SDK PR (shadow demo and integration tests), until or unless this API is accepted.

Note: Doxygen pipeline check is failing because Doxygen is not installing. Nothing to do with this PR I believe.

source/include/shadow.h Outdated Show resolved Hide resolved
source/include/shadow.h Outdated Show resolved Hide resolved
source/include/shadow.h Outdated Show resolved Hide resolved
source/include/shadow.h Outdated Show resolved Hide resolved
@gregbreen gregbreen force-pushed the feature/named-shadows branch from 7faa10f to c907b13 Compare January 15, 2021 01:37
@gregbreen
Copy link
Contributor Author

OK. After considerable additional offline conversation, re-worked the backwards compatibility.

  • Full support of all existing macros.
  • Retain the concept of one convenience function-like macro for each operation-suffix. (but now with a shadowName parameter)
  • Ensure all new macros names are 31 characters or fewer (to ensure no issues with MISRA 5.1).

New function example (with shadowName parameter):

image

Deprecated function example:

image

New function-like macro example (with shadowName parameter):

image

Deprecated function-like macro example:

image

Note: The CI/CD Doxygen check is still failing. It's because https://www.doxygen.nl/files/doxygen-1.8.20.linux.bin.tar.gz is giving a 404. Looks like this will affect lots of the SDK repos.

source/include/shadow.h Outdated Show resolved Hide resolved
source/include/shadow.h Outdated Show resolved Hide resolved
Co-authored-by: leegeth <51681119+leegeth@users.noreply.github.com>
@leegeth leegeth merged commit 14c37f6 into aws:main Jan 20, 2021
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.

4 participants