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

gcoap: add helper function to get request header from a request memo #18095

Merged
merged 1 commit into from May 12, 2022

Conversation

miri64
Copy link
Member

@miri64 miri64 commented May 12, 2022

Contribution description

Just some code-deduplication.

Testing procedure

examples/gcoap should still compile with and without the gcoap_forward_proxy.

Issues/PRs references

Spotted by @cgundogan in #17801 but can be merged independent of it (but might cause merge conflicts there or here).

@miri64 miri64 added the Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation label May 12, 2022
@github-actions github-actions bot added Area: CoAP Area: Constrained Application Protocol implementations Area: network Area: Networking Area: sys Area: System labels May 12, 2022
sys/include/net/gcoap.h Outdated Show resolved Hide resolved
@cgundogan cgundogan self-requested a review May 12, 2022 11:31
Copy link
Member

@cgundogan cgundogan left a comment

Choose a reason for hiding this comment

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

ACK. Please squash. The linting errors are actually unrelated.

@cgundogan cgundogan added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines labels May 12, 2022
@miri64 miri64 force-pushed the gcoap/enh/get-req-hdr-from-memo branch from a733843 to 584681f Compare May 12, 2022 11:47
* @return The request header for the given request memo.
*/
static inline coap_hdr_t *gcoap_request_memo_get_hdr(const gcoap_request_memo_t *memo)
{
Copy link
Member

Choose a reason for hiding this comment

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

Not critical: what's your opinion on adding an assert(memo != NULL)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Would require to include assert.h and also, since the macro uses RIOT_FILE_RELATIVE the provided line would be wrong (it would point to the including C-file). That's why I intentially omitted it.

Copy link
Member

Choose a reason for hiding this comment

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

On the premise that the compiler really inlines this inline function ..

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it will still take the line and file from code.

Copy link
Member Author

Choose a reason for hiding this comment

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

But RIOT_FILE_RELATIVE is broken wtr, not the compiler ;-).

@cgundogan cgundogan enabled auto-merge May 12, 2022 12:19
@miri64 miri64 force-pushed the gcoap/enh/get-req-hdr-from-memo branch from 584681f to e9a12b7 Compare May 12, 2022 13:12
@miri64
Copy link
Member Author

miri64 commented May 12, 2022

Rebased, after #17801 got merged.

@miri64 miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 12, 2022
@miri64 miri64 force-pushed the gcoap/enh/get-req-hdr-from-memo branch from e9a12b7 to cbbde07 Compare May 12, 2022 13:22
@miri64
Copy link
Member Author

miri64 commented May 12, 2022

Found another occurance.

Copy link
Member

@cgundogan cgundogan left a comment

Choose a reason for hiding this comment

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

Still looks good. ACK!

@cgundogan cgundogan merged commit 11b3121 into RIOT-OS:master May 12, 2022
@miri64 miri64 deleted the gcoap/enh/get-req-hdr-from-memo branch May 12, 2022 15:35
@chrysn chrysn added this to the Release 2022.07 milestone Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: CoAP Area: Constrained Application Protocol implementations Area: network Area: Networking Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants