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

Simplify tmp directory computation for extern controllers #6002

Merged
merged 29 commits into from
Mar 24, 2023

Conversation

ygoumaz
Copy link
Contributor

@ygoumaz ygoumaz commented Mar 17, 2023

Fixes #5300.

The wbu_system_dir() function is only used to get the tmp dir when using ipc extern controllers. The WEBOTS_TMPDIR is not useful and can be discarded in this case. Instead we use WEBOTS_HOME to determine if we target a snap installation of Webots. If not we default to /tmp.

This PR also updates the test_sources to Ubuntu 22 to fix some cppcheck false positives. The clang-format version is locked to 14.0.0.

@ygoumaz ygoumaz added the bug Something isn't working label Mar 17, 2023
@ygoumaz ygoumaz self-assigned this Mar 17, 2023
@ygoumaz ygoumaz added the test sources Start the sources test on all platforms label Mar 17, 2023
@ygoumaz ygoumaz added test sources Start the sources test on all platforms and removed test sources Start the sources test on all platforms labels Mar 20, 2023
@ygoumaz ygoumaz marked this pull request as ready for review March 21, 2023 11:02
@ygoumaz ygoumaz requested a review from a team as a code owner March 21, 2023 11:02
@ygoumaz ygoumaz requested a review from omichel March 23, 2023 12:12
Copy link
Member

@omichel omichel left a comment

Choose a reason for hiding this comment

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

Looks good to me.
Thank you.

@ygoumaz ygoumaz merged commit 60c4e81 into master Mar 24, 2023
@ygoumaz ygoumaz deleted the fix-extern-controller-tmp-dir branch March 24, 2023 14:52
Comment on lines +107 to +108
const bool clampNeeded = WbRgb(cr, cg, cb).clampValuesIfNeeded();
assert(!clampNeeded);
Copy link
Member

Choose a reason for hiding this comment

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

WbRobot::WbRobot(const WbRobot &other) : WbSolid(other) {
WbRobot::WbRobot(const WbRobot &other) :
WbSolid(other),
mControllerDir(),
Copy link
Member

@stefaniapedrazzi stefaniapedrazzi Mar 27, 2023

Choose a reason for hiding this comment

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

All these initialization lines are useless and should be removed because QString objects are already initialized by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cppcheck triggers an error if they are missing.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. But the cppcheck error/warning is wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if using a lot of suppress tags for cppcheck is really the best solution. Is it really that bad to initialize those QString objects?

Copy link
Member

@stefaniapedrazzi stefaniapedrazzi Mar 27, 2023

Choose a reason for hiding this comment

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

It is completely useless to explicitly initialize the strings and all the other Qt objects to default values.
From my point of view it makes the code less readable than before because we have a long list of useless instructions.
I think that cppcheck should help us writing better code, not worse and lately it is not always the case.
We should try to adjust the configuration to try to avoid these false positives.

Comment on lines +88 to +97
mInitialSkeletonOrientation(),
mInitialSkeletonPosition(),
mRenderables(),
mMaterialNames(),
mMaterials(),
mSegmentationMaterials(),
mEncodeDepthMaterials(),
mMeshes(),
mBoneTransforms(),
mBonesMap() {
Copy link
Member

@stefaniapedrazzi stefaniapedrazzi Mar 27, 2023

Choose a reason for hiding this comment

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

The newly added initialization lines should be removed because they are useless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cppcheck triggers an error if they are missing.

@@ -117,7 +117,7 @@ WbReceiver::WbReceiver(WbTokenizer *tokenizer) : WbSolidDevice("Receiver", token
init();
}

WbReceiver::WbReceiver(const WbReceiver &other) : WbSolidDevice(other) {
WbReceiver::WbReceiver(const WbReceiver &other) : WbSolidDevice(other), mTransmissionList(), mWaitingQueue(), mReadyQueue() {
Copy link
Member

Choose a reason for hiding this comment

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

The newly added initialization instructions are useless and should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cppcheck triggers an error if they are missing.

@@ -134,7 +134,7 @@ WbLightSensor::WbLightSensor(WbTokenizer *tokenizer) : WbSolidDevice("LightSenso
init();
}

WbLightSensor::WbLightSensor(const WbLightSensor &other) : WbSolidDevice(other) {
WbLightSensor::WbLightSensor(const WbLightSensor &other) : WbSolidDevice(other), mRayList() {
Copy link
Member

Choose a reason for hiding this comment

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

It useless to explicitly initialize mRayList to the default value.

@@ -38,7 +38,7 @@ WbLed::WbLed(WbTokenizer *tokenizer) : WbSolidDevice("LED", tokenizer) {
init();
}

WbLed::WbLed(const WbLed &other) : WbSolidDevice(other) {
WbLed::WbLed(const WbLed &other) : WbSolidDevice(other), mMaterials(), mPbrAppearances(), mLights() {
Copy link
Member

@stefaniapedrazzi stefaniapedrazzi Mar 27, 2023

Choose a reason for hiding this comment

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

here as well, the new initialization instructions are useless.
Also in WbDisplay, WbCharger, WbAbstractCamera,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working test sources Start the sources test on all platforms
Development

Successfully merging this pull request may close these issues.

Search for Webots tmp directory in libController is not robust
3 participants