Fix some warnings #2682
Fix some warnings #2682
Conversation
| @@ -54,7 +54,7 @@ macro(CommonSetup) | |||
| else () | |||
| set(CMAKE_CXX_FLAGS "\ | |||
| -std=c++17 -ggdb -Wall -Wextra \ | |||
| -Wno-variadic-macros -Wno-parentheses -Wno-unused-function -Wno-unused \ | |||
| -Wno-variadic-macros -Wno-parentheses -Wno-unused-function -Wno-unused -Wno-unknown-warning-option \ | |||
There was a problem hiding this comment.
Should we remove the -Wno-unused flag here?
There was a problem hiding this comment.
-Wno-unused is meant to ignore warnings involving unused variables, are you seeing those even with this flag on?
There was a problem hiding this comment.
No, the warnings are coming from the Unity build where it hasn't been disabled
I'm not sure if we should disable them or not, I haven't yet fixed all the warnings
|
I would advise against removing variables, especially in complex libraries like MavlinkCom - while they may be unused in that particular piece of code, they could have been meant to exist for the purpose of extensibility or some sort of parity. If a variable has a purpose and is named appropriately it does not detract from the code as a whole. Warnings are a smaller problem, and I recommend we handle it through compiler flags. My two cents |
|
I agree, removing all the unused variables probably doesn't make much sense, that's why I haven't yet removed variables from places like AdaptiveController and some others in Mavlinkcom |
|
Once the warnings which should be ignored and fixed are decided, then will clean up the commits I'll go through the Mavlinkcom commits and only remove variables which seem harmless, or don't have any meaning currently |
|
I think I'll add back the other variables in Mavlinkcom but remove the ones commented above if others agree |
|
Commits cleaned up, only removed variables which didn't fit in the context |
rajat2004
force-pushed
the
warnings
branch
2 times, most recently
from
5e0ed2e to
5b4d524
Compare
|
|
||
| // Already have goal, and have reached it | ||
| ROS_INFO_STREAM("[PIDPositionController] Already have goal and have reached it"); | ||
| return false; |
There was a problem hiding this comment.
I'm not sure if this is correct or not, we should probably set the goal in this case also
|
/azp run microsoft.AirSim |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There are more warnings, many unused variables which can be seen when running the Unity build
Right now only testing till compilation