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

cxxtest: optional stream test #2722

Merged
merged 1 commit into from
Oct 15, 2024
Merged

Conversation

cuiziwei1
Copy link

@cuiziwei1 cuiziwei1 commented Oct 14, 2024

Summary

skip the locale related test if libc doesn't enable CONFIG_CXX_LOCALIZATION.

Impact

cxxtest

Testing

ci

@nuttxpr
Copy link

nuttxpr commented Oct 14, 2024

[Experimental Bot, please feedback here]

I need the content of the PR to assess if it meets the requirements. Please provide the following:

  • The actual changes made in the PR: A link to the PR or a description of the code modifications.
  • The PR description: This usually contains information about the purpose and details of the changes.

With this information, I can then evaluate the PR against the NuttX requirements and provide you with a concise answer.

Copy link

@cederom cederom left a comment

Choose a reason for hiding this comment

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

This is CPP test, you cannot remove C++ specific calls here, rejecting.

PR without description will be rejected, see https://github.com/apache/nuttx/blob/master/CONTRIBUTING.md.

If you think otherwise please reopen and explain the concept in detail.

@cederom cederom closed this Oct 14, 2024
@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented Oct 14, 2024

This is CPP test, you cannot remove C++ specific calls here, rejecting.

why? locale is an optional feature in nuttx libc, so c++ test code should skip locale related test if libc close this feature.
BTW, the test case still keep and run if you enable CONFIG_CXX_LOCALIZATION, so I don't see any reason to reject this patch.

PR without description will be rejected, see https://github.com/apache/nuttx/blob/master/CONTRIBUTING.md.

If you think otherwise please reopen and explain the concept in detail.

Update.

@cederom
Copy link

cederom commented Oct 14, 2024

Thanks @xiaoxiang781216 :-)

It is reaaaallly important to write good description of the PR, there was none. Contributing Guidelines were updated to provide examples, there is a reference at top of PR report, we even created PR AI BOT to suggest how the description can be improved to help in quick/good assessment. We have a lot of PR to analyze so we need the description why change is necessary, what it fixes/adds, what is the testing result before and after, so PR dont break more than it fixes. We cannot blindly accept undocumented PRs because things start falling apart :-)

C++ NuttX maintainers would be the best person to ask for a review here, I don't work with C++ sorry, if they give a GO you have my GO too. I have requested a review support on dev@ mailing list :-)

What I don't like here is replacing std::cout (C++ way) with printf (C way) and there is no explanation why this is necessary. I know C++ folks don't like this kind of stuff. It was here before for a reason right? :-)

@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented Oct 14, 2024

Thanks @xiaoxiang781216 :-)

It is reaaaallly important to write good description of the PR, there was none. Contributing Guidelines were updated to provide examples, there is a reference at top of PR report, we even created PR AI BOT to suggest how the description can be improved to help in quick/good assessment. We have a lot of PR to analyze so we need the description why change is necessary, what it fixes/adds, what is the testing result before and after, so PR dont break more than it fixes. We cannot blindly accept undocumented PRs because things start falling apart :-)

Yes, it always good to request the contributor to add the detailed description.

C++ NuttX maintainers would be the best person to ask for a review here, I don't work with C++ sorry, if they give a GO you have my GO too :-)

Most C++ change in NuttX contributes by my team, so I am a good candidate to review the related change.

What I don't like here is replacing std::cout (C++ way) with printf (C way) and there is no explanation why this is necessary.

Because cin/cout depends on locale to pass the build, that's why to replace the code which call cin/cout unconditionally, the conditional part still keep as before:
https://github.com/apache/nuttx-apps/pull/2722/files#diff-9d02d89b087be466dd6bbdf41f260690b28365e09a162260f69e20ce7a4e2896R110
https://github.com/apache/nuttx-apps/pull/2722/files#diff-9d02d89b087be466dd6bbdf41f260690b28365e09a162260f69e20ce7a4e2896R141

I know C++ folks don't like this kind of stuff. It was here before for a reason right? :-)

cout/cin is very code size consumption:
https://github.com/apache/nuttx-apps/blob/master/testing/cxxsize/README.md
it's good to avoid iostream in embedded space.

here is the patch to reduce libcxx code size:
apache/nuttx#13721
apache/nuttx#14244
apache/nuttx#14250

Copy link

@cederom cederom left a comment

Choose a reason for hiding this comment

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

Thank you @xiaoxiang781216 for the detailed explanation, now all clear :-)

  • C++ code wrapped conditionally and executed properly when enabled.
  • printf in place of std:: to reduce embedded code size where possible.

You see two sentences of explanation and things look totally different :-) :-)

@xiaoxiang781216
Copy link
Contributor

Yes, @cuiziwei1 please update the pr, it's always important to describe the change clearly.

@cuiziwei1 cuiziwei1 force-pushed the 2024101401 branch 2 times, most recently from 52df72d to d54f350 Compare October 15, 2024 08:27
Signed-off-by: jihandong <[email protected]>
Signed-off-by: cuiziwei <[email protected]>
@xiaoxiang781216 xiaoxiang781216 merged commit eca0928 into apache:master Oct 15, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants