Add available macros for 'return error code' as an alterative to 'verifyorexit' #3880

andy31415 · 2020-11-17T16:55:05Z

Problem

Existing coding pattern of 'exit:' goto label is convenient for cleanup code, however it encourages C-style coding which has drawbacks:

less use of RAII (typical C++ cleanup)
forces variable declarations to be at the top of the scope due to the goto statement, moving variables further away from their usage.

Summary of Changes

Creates Verify/return macros that are equivalent to the 'exit' macros and converts some files with sample usage for these macros.

Generated code is generally the same size as the previous goto code (with some slight immaterial improvements) and source code is shorter.

Convert a few more rendezvous to return macros instead of verify - seems 0 code size difference

rwalker-apple

yay! on our way to no labels?

andy31415 · 2020-11-17T18:56:29Z

yay! on our way to no labels?

I am not a fan of goto. I understand it is useful for embedded, but it should not be forced on me. I would like everyone to have a choice.

src/lib/support/ReturnMacros.h

src/transport/RendezvousSession.cpp

bzbarsky-apple · 2020-11-17T22:55:04Z

src/transport/raw/TCP.cpp


    if (endPoint != nullptr)
    {
-        err = endPoint->Send(msgBuf);
+        return endPoint->Send(autofree.Release());
    }
    else


What's our general position on else after return?

I generally prefer single point of return or exit / error handling (RAII or not) which moots this particular facet of coding style discussion.

I have created this PR after seeing that mandating goto with single point of exit can result in significantly hard to follow code.

The premise is that we will have the option to use return if the code is cleaner as a result. Mandating exit label always seems too strong of a requirement.

Regarding the else after return: I think generally I would say no else is needed after return. In this particular case however, this is a final statement that reads like:

return (endpoint != nullptr) ? endPoint->Send(autofree.Release()) : SendAfterConnect(address, autofree.Release());

so I thought else helps clarity. I would drop the else if the readability reviews would ask for it.

I support @gerickson. Probably it's a matter of taste and it will be hard to decide on "the right" approach. For me this change makes code less readable and more error-prone (especially if we'll start mixing e.g. VerifyOrExit with VerifyOrReturn in a single method). But maybe it's just my embedded background..

Of course giving the choice for developers is good, though we lose consistency when both approaches are used in different places.

+1 that mixing in a single method would be absolutely bad - you get the benefit of neither. I am not arguing for that. I am arguing that I should be allowed to use return if it makes the code easier/clearer to read.

I could not find a good way to count, but find . -type f | egrep -v '/third_party/' | xargs egrep -zc 'exit:\s*return' | grep -v ':0' | wc -l seems to indicate that there are 149 instances today where 'exit: return...' is used. I believe those instances would benefit from just using return in the first place.

C forces you into the goto exit pattern, but C++ doesn't, and the goto style with declarations front loaded seems unusual for C++. Google's style guide doesn't even bother to recommend against using goto because it is so uncommon in the first place [1].

[1] https://google.github.io/styleguide/cppguide.html#Goals

github-actions · 2020-11-19T15:30:11Z

Size increase report for "nrfconnect-example-build" from 188a35f

File	Section	File	VM
chip-shell.elf	rodata	-40	-40
chip-shell.elf	text	-2448	-2448
chip-lighting.elf	rodata	-32	-36
chip-lighting.elf	text	-2496	-2496
chip-lock.elf	rodata	-40	-40
chip-lock.elf	text	-2496	-2496

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize
.shstrtab,0,1
.debug_aranges,0,-8
rodata,-40,-40
.debug_frame,0,-44
.debug_ranges,0,-72
.debug_str,0,-81
.debug_line,0,-405
.debug_loc,0,-515
.strtab,0,-769
.debug_info,0,-779
.symtab,0,-1088
text,-2448,-2448

Comparing ./master_artifact/chip-lighting.elf and ./pull_artifact/chip-lighting.elf:

sections,vmsize,filesize
.debug_ranges,0,240
.shstrtab,0,-2
.debug_aranges,0,-8
.debug_abbrev,0,-35
rodata,-36,-32
.debug_frame,0,-52
.debug_str,0,-82
.debug_info,0,-373
.debug_line,0,-720
.strtab,0,-762
.symtab,0,-1088
.debug_loc,0,-1114
text,-2496,-2496

Comparing ./master_artifact/chip-lock.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize
.debug_ranges,0,240
.shstrtab,0,-2
.debug_aranges,0,-8
.debug_abbrev,0,-35
rodata,-40,-40
.debug_frame,0,-52
.debug_str,0,-82
.debug_info,0,-373
.debug_line,0,-720
.strtab,0,-762
.symtab,0,-1088
.debug_loc,0,-1106
text,-2496,-2496

github-actions · 2020-11-19T15:39:56Z

Size increase report for "esp32-example-build" from 188a35f

File	Section	File	VM
chip-all-clusters-app.elf	.flash.rodata	-4	-4
chip-all-clusters-app.elf	.flash.text	-4	-4
chip-all-clusters-app.elf	.dram0.data	-8	-8

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.debug_info,0,319
.debug_ranges,0,104
.debug_abbrev,0,49
.debug_frame,0,24
[Unmapped],0,12
.debug_aranges,0,8
.strtab,0,7
.shstrtab,0,-3
.flash.rodata,-4,-4
.flash.text,-4,-4
.debug_str,0,-7
.dram0.data,-8,-8
.debug_loc,0,-76
.debug_line,0,-149

mspang · 2020-11-19T18:35:15Z

src/transport/raw/TCP.cpp


    if (endPoint != nullptr)
    {
-        err = endPoint->Send(msgBuf);
+        return endPoint->Send(autofree.Release());
    }
    else


C forces you into the goto exit pattern, but C++ doesn't, and the goto style with declarations front loaded seems unusual for C++. Google's style guide doesn't even bother to recommend against using goto because it is so uncommon in the first place [1].

[1] https://google.github.io/styleguide/cppguide.html#Goals

Start adding some return macro support instead of verifyorexit

f197402

Convert a few more rendezvous to return macros instead of verify - seems 0 code size difference

boring-cyborg bot added lib transport labels Nov 17, 2020

restyled-io bot mentioned this pull request Nov 17, 2020

Restyle Add available macros for 'return error code' as an alterative to 'verifyorexit' #3881

Closed

andy31415 added 2 commits November 17, 2020 11:57

Remove useless return (no more goto label)

e27f29c

Restyler fixes

85382d9

pullapprove bot requested review from BroderickCarlin, bzbarsky-apple, chrisdecenzo, hawk248, jelderton, mspang, rwalker-apple and saurabhst November 17, 2020 16:59

jelderton approved these changes Nov 17, 2020

View reviewed changes

rwalker-apple approved these changes Nov 17, 2020

View reviewed changes

bzbarsky-apple reviewed Nov 17, 2020

View reviewed changes

src/lib/support/ReturnMacros.h Outdated Show resolved Hide resolved

src/lib/support/ReturnMacros.h Outdated Show resolved Hide resolved

src/lib/support/ReturnMacros.h Outdated Show resolved Hide resolved

src/transport/RendezvousSession.cpp Outdated Show resolved Hide resolved

bzbarsky-apple reviewed Nov 17, 2020

View reviewed changes

bzbarsky-apple mentioned this pull request Nov 18, 2020

Implement cluster controller base class and port OnOff cluster to it #3879

Merged

andy31415 added 2 commits November 18, 2020 10:41

Update based on code review comments

7b27e51

Merge branch 'master' into 01_return_macros

c728420

andy31415 mentioned this pull request Nov 18, 2020

Some conversions to use PacketBufferHandle #3909

Merged

bzbarsky-apple approved these changes Nov 19, 2020

View reviewed changes

andy31415 added 2 commits November 19, 2020 10:10

Merge branch 'master' into 01_return_macros

2df3691

Fix merge issues

2e9a09c

mspang approved these changes Nov 19, 2020

View reviewed changes

rwalker-apple merged commit d513b63 into project-chip:master Nov 19, 2020

andy31415 deleted the 01_return_macros branch October 28, 2021 13:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add available macros for 'return error code' as an alterative to 'verifyorexit' #3880

Add available macros for 'return error code' as an alterative to 'verifyorexit' #3880

andy31415 commented Nov 17, 2020

rwalker-apple left a comment

andy31415 commented Nov 17, 2020

bzbarsky-apple Nov 17, 2020

gerickson Nov 17, 2020

andy31415 Nov 18, 2020

andy31415 Nov 18, 2020

LuDuda Nov 18, 2020

andy31415 Nov 19, 2020

andy31415 Nov 19, 2020

mspang Nov 19, 2020

github-actions bot commented Nov 19, 2020

github-actions bot commented Nov 19, 2020

mspang Nov 19, 2020

Add available macros for 'return error code' as an alterative to 'verifyorexit' #3880

Add available macros for 'return error code' as an alterative to 'verifyorexit' #3880

Conversation

andy31415 commented Nov 17, 2020

Problem

Summary of Changes

rwalker-apple left a comment

Choose a reason for hiding this comment

andy31415 commented Nov 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 19, 2020

github-actions bot commented Nov 19, 2020

Choose a reason for hiding this comment