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

Fix unused variable warning issues #2357

Merged
merged 13 commits into from
Sep 2, 2022
Merged

Conversation

JoseSanchez7
Copy link
Contributor

@JoseSanchez7 JoseSanchez7 commented Aug 24, 2022

Description of the problem:

Warnings of unused variables were being produced upon compilation. See link to issue below:

Link: Azure/azure-c-shared-utility#599

Description of the solution:

Replaced instances of "static char *" with "#define" for certain string constants in include files. Also, deleted variables that weren't used elsewhere in the code.

@@ -379,7 +379,6 @@ Actions are discarded, since no marshalling will be done for those when sending
{ \
AGENT_DATA_TYPES_RESULT result = AGENT_DATA_TYPES_OK; \
size_t iMember = 0; \
DEFINITION_THAT_CAN_SUSTAIN_A_COMMA_STEAL(phantomName, 1); \
Copy link
Member

Choose a reason for hiding this comment

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

what issues were these presenting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I recall correctly, it was that they weren't being used. I checked other files to confirm.

Copy link
Contributor

Choose a reason for hiding this comment

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

please have JohnS and Ewerton review these changes in this file

Copy link
Contributor

Choose a reason for hiding this comment

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

;tl-dr - this is safe to remove.

From the definition of DEFINITION_THAT_CAN_SUSTAIN_A_COMMA_STEAL (at bottom of this file) is a workaround for a VS2005 preprocessor bug.

We don't support VS2005.

A good thing with this is that if any other preprocessors had this bug that needed this workaround, we'd hit it very early in compilation process. The actual data here is by design meant to be compiled out and have 0 runtime impact.

CMakeLists.txt Outdated Show resolved Hide resolved
Co-authored-by: Dane Walton <dane.walton@microsoft.com>
CMakeLists.txt Outdated
@@ -131,6 +131,8 @@ if (${warnings_as_errors})
# Make warning as error
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Werror")
set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Werror")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wunused-variable -Wmaybe-uninitialized")
Copy link
Member

Choose a reason for hiding this comment

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

You might need to differentiate between Clang and GCC. Clang looks like it might not have -Wmaybe-uninitialized.

Copy link
Contributor

Choose a reason for hiding this comment

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

..and OSX

@danewalton
Copy link
Member

image

Adding suggestion here for putting the compilation in the Linux with Options so that we don't get the bad warnings from all the tests.

Co-authored-by: Dane Walton <dane.walton@microsoft.com>
@JoseSanchez7 JoseSanchez7 merged commit 1b1db47 into main Sep 2, 2022
@JoseSanchez7 JoseSanchez7 deleted the jose/fix_warning_errors branch September 2, 2022 18:40
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.

4 participants