Skip to content

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

Merged
cgundogan merged 1 commit intoRIOT-OS:masterfrom
miri64:gcoap/enh/get-req-hdr-from-memo
May 12, 2022
Merged

gcoap: add helper function to get request header from a request memo#18095
cgundogan merged 1 commit intoRIOT-OS:masterfrom
miri64:gcoap/enh/get-req-hdr-from-memo

Conversation

@miri64
Copy link
Copy Markdown
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
cgundogan
cgundogan reviewed May 12, 2022
View reviewed changes
@cgundogan cgundogan self-requested a review May 12, 2022 11:31
cgundogan
cgundogan approved these changes May 12, 2022
View reviewed changes
Copy link
Copy Markdown
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
cgundogan
cgundogan reviewed May 12, 2022
View reviewed changes
sys/include/net/gcoap.h
* @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
Copy Markdown
Member

@cgundogan cgundogan May 12, 2022

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
Copy Markdown
Member Author

@miri64 miri64 May 12, 2022

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
Copy Markdown
Member

@cgundogan cgundogan May 12, 2022

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
Copy Markdown
Member Author

@miri64 miri64 May 12, 2022

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
Copy Markdown
Member Author

@miri64 miri64 May 12, 2022

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
Copy Markdown
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
Copy Markdown
Member Author

miri64 commented May 12, 2022

Found another occurance.

@miri64 miri64 requested a review from cgundogan May 12, 2022 13:22
@miri64 miri64 mentioned this pull request May 12, 2022
Merged
cgundogan
cgundogan approved these changes May 12, 2022
View reviewed changes
Copy link
Copy Markdown
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

Reviewers

@cgundogan cgundogan cgundogan approved these changes

@haukepetersen haukepetersen Awaiting requested review from haukepetersen

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

Assignees

No one assigned

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

Milestone

Release 2022.07

Development

Successfully merging this pull request may close these issues.

3 participants

@miri64 @cgundogan @chrysn