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

This removes many const char* warnings related with LWIP_ASSERT() #10500

Merged
merged 1 commit into from May 12, 2019

Conversation

Projects
None yet
7 participants
@andrewc-arm
Copy link
Contributor

commented Apr 27, 2019

Description

If we enable LWIP debugging option like "lwip.debug-enabled": true,, we get many warnings related with LWIP_ASSERT like following.

[Warning] cc.h@117,78: deprecated conversion from string constant to 'char*' [-Wwrite-strings]
[Warning] cc.h@117,78: deprecated conversion from string constant to 'char*' [-Wwrite-strings]
[Warning] cc.h@117,78: deprecated conversion from string constant to 'char*' [-Wwrite-strings]

For more information, please refer to the attached build_log.txt file.
build_log.txt

The code change removes duplicate warnings.
This problem was reproduced in GCC_ARM and fixed in GCC_ARM. Other toolchains are not tested.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

Release Notes

@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Apr 27, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

@andrewc-arm, thank you for your changes.
@ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

@kjbracey-arm
Copy link
Contributor

left a comment

Fine, but I'm curious where you're seeing the warnings. Which toolchain? Have you enabled extra warnings?

The lack of const here would be a problem in C++, where string literals are const char[], but in C they're char[], so passing a string literal to this is valid, and I don't recall seeing any warnings myself.

@0xc0170
Copy link
Member

left a comment

Missing description and PR type in the first comment (PR template) - please fill in the details

As requested, how was this tested - which toolchain produce warnings?

@0xc0170 0xc0170 added needs: work and removed needs: review labels Apr 29, 2019

@adbridge

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

@andrewc-arm please correctly fill in the PR template header. Needs a proper description and pull request type setting

@andrewc-arm

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

Sorry for the late response. I have been working on Samsung issues lately. I will get back soon.

@andrewc-arm

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

Hi, @adbridge and @0xc0170
I updated the PR template. Could you please let me what more is needed?

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

My question about where/why you were seeing warnings remains outstanding.

@andrewc-arm

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

Hi, @kjbracey-arm

My template comment far above has been updated.

If we enable LWIP debugging option like "lwip.debug-enabled": true,, we get many warnings related with LWIP_ASSERT like following.

[Warning] cc.h@117,78: deprecated conversion from string constant to 'char*' [-Wwrite-strings]
[Warning] cc.h@117,78: deprecated conversion from string constant to 'char*' [-Wwrite-strings]
[Warning] cc.h@117,78: deprecated conversion from string constant to 'char*' [-Wwrite-strings]

For more information, please refer to the attached build_log.txt file.
build_log.txt

I thought the log and error messages are good enough. If not, please let me know.

@adbridge

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

@kjbracey-arm are you happy with this ?

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

Sorry, missed part of your update.

So the issue is your own ping.cpp file? I guess you've taken lwIP's ping.c, and are compiling it as C++?

Okay, that would explain why I've not seen it - that LWIP_ASSERT would have only ever been used from C code - it's your application's attempt to use it from C++ that's generating the warnings.

Yes, fix is fine - makes it safe for C++ use.

@0xc0170

0xc0170 approved these changes May 2, 2019

@0xc0170 0xc0170 added needs: CI and removed needs: work labels May 2, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented May 2, 2019

I'll schedule CI a bit later today due to 5.12.3 final patches in CI

@andrewc-arm

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

Hi, @kjbracey-arm

So the issue is your own ping.cpp file? I guess you've taken lwIP's ping.c, and are compiling it as C++?

I am not so sure where the ping.c came from. I got the original ping.cpp file from Samsung's application code which seems to be Apache 2 style license. I improved it for IPv6 in mbed-os-example-ping. Thanks for approval!

@adbridge

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented May 7, 2019

Test run: FAILED

Summary: 2 of 11 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_dynamic-memory-usage
  • jenkins-ci/mbed-os-ci_greentea-test
@andrewc-arm

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

Hi, @adbridge

This is excerpt of the failure.

[1557254704.66][CONN][INF] found KV pair in stream: {{start;1}}, queued...
[1557254704.66][HTST][INF] host test class: '<class 'rtc_reset.RtcResetTest'>'
[1557254704.66][HTST][ERR] something went wrong in event main loop!
[1557254704.66][HTST][INF] ==== Traceback start ====
Traceback (most recent call last):
File "/usr/local/lib/python3.6/dist-packages/mbed_os_tools-0.0.8-py3.6.egg/mbed_os_tools/test/host_tests_runner/host_test_default.py", line 306, in run_test
self.test_supervisor.setup()
File "/builds/ws/mbed-os-ci_greentea-test@7/mbed-os/TESTS/host_tests/rtc_reset.py", line 43, in setup
generator.next()
AttributeError: 'generator' object has no attribute 'next'
[1557254704.66][HTST][INF] ==== Traceback end ====

I think this failure is not caused by my simple code change in LwIP. Or am I missing something?

@adbridge

This comment has been minimized.

Copy link
Contributor

commented May 8, 2019

@andrewc-arm Looks like there were some changes to the CI which broke the test. We will re-run the tests once they have fixed it.

@0xc0170

This comment has been minimized.

Copy link
Member

commented May 10, 2019

CI restarted

@mbed-ci

This comment has been minimized.

Copy link

commented May 10, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 2
Build artifacts

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels May 12, 2019

@0xc0170 0xc0170 merged commit 03cda26 into ARMmbed:master May 12, 2019

26 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage Success
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8458 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8448B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.