Skip to content

netutils/webclient: Fix bug that the socket is not close#2501

Merged
xiaoxiang781216 merged 2 commits intoapache:masterfrom
SPRESENSE:fix-socket-not-close
Aug 20, 2024
Merged

netutils/webclient: Fix bug that the socket is not close#2501
xiaoxiang781216 merged 2 commits intoapache:masterfrom
SPRESENSE:fix-socket-not-close

Conversation

@SPRESENSE
Copy link
Copy Markdown
Contributor

Summary

Fix to close socket when it fails to resolve hostname.

Impact

webclient.

Testing

Test with Spresense LTE board.

Fix to close socket when it fails to resolve hostname.
Copy link
Copy Markdown
Contributor

@jerpelea jerpelea left a comment

Choose a reason for hiding this comment

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

please fix the commit

@xiaoxiang781216
Copy link
Copy Markdown
Contributor

Please fix the style issue, @SPRESENSE

@SPRESENSE
Copy link
Copy Markdown
Contributor Author

@jerpelea @xiaoxiang781216
I have fixed the style issue. Please check.

@xiaoxiang781216
Copy link
Copy Markdown
Contributor

xiaoxiang781216 commented Aug 20, 2024

@acassis and @LuchianMihai does apache/nuttx#12794 change good? The style of original code looks perfect.

@tinnedkarma
Copy link
Copy Markdown
Contributor

tinnedkarma commented Aug 20, 2024

Hi @xiaoxiang781216 , @acassis

Strictly regarding the C Coding Standard, is stated that the:

Comments to the right of statements in C source files are discouraged.

Indentation Comments should, typically, be placed before the code section to which they apply. The comment indentation should be the same as the follow indentation rules as the following code (if applicable).

Now there is nothing explicitly stating that there should not be comments to the right of if statements, but I saw no comment at the right of the statement throughout the examples given there (in C Coding Standard page).

My change will report an error if there is anything to the right of the statement, because checking that the comment is correctly formatted (as the comment should be indented accordingly) would be harder, and maybe not worth the time penalty for checking that corner case. Maybe we can report this as an warning?

@xiaoxiang781216
Copy link
Copy Markdown
Contributor

Hi @xiaoxiang781216 , @acassis

Strictly regarding the C Coding Standard, is stated that the:

Comments to the right of statements in C source files are discouraged.

Indentation Comments should, typically, be placed before the code section to which they apply. The comment indentation should be the same as the follow indentation rules as the following code (if applicable).

Now there is nothing explicitly stating that there should not be comments to the right of if statements, but I saw no comment at the right of the statement throughout the examples given there (in C Coding Standard page).

@patacongo @acassis what's your opinion?

My change will report an error if there is anything to the right of the statement, because checking that the comment is correctly formatted (as the comment should be indented accordingly) would be harder, and maybe not worth the time penalty for checking that corner case. Maybe we can report this as an warning?

The code base is large, new check will break many codes which pass the ci before. So, we need be more conservative here.

@xiaoxiang781216 xiaoxiang781216 merged commit fb17471 into apache:master Aug 20, 2024
@tinnedkarma
Copy link
Copy Markdown
Contributor

Just run a quick search through the codebase:

grep -r "if (.*)\ */*" | wc -l # This reports the total number of occurrences: 78

grep -r "if (.*)\ */*" -l | wc -l # This reports the total number of independent files affected: 32

I'm only counting correctly formatted if statements which are followed by spaces until the beginning of a comment is found. We add a few more changes if I include incorrectly formatted if statements.

But nevertheless, it's quite a big surface area as it is, maybe my changes are not adding more value than adding issues.

@acassis
Copy link
Copy Markdown
Contributor

acassis commented Aug 20, 2024

@LuchianMihai maybe it should be reported as warning instead of error

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.

5 participants