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

fixed some problems, when compiling with very strict compile options #13

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BruceBlank
Copy link

A have split the original commit in to two: This fix will avoid warnings when compiling

@BruceBlank
Copy link
Author

Hi all,

you can think of this fix as a sort of code-style-correction, But as this projects code will be bound into other code by an #include directive, it is more than that. If you work on a project, which demands very strict compile options (-Wall -Wextra -Wshadow -Werror), as i do, you have the problem, that the code will not compile, without this fix or without another work around.

greetings
Bruce

@toch
Copy link
Collaborator

toch commented May 7, 2016

Thanks @BruceBlank. Sorry for the delay.

I understand your case. However, code convention is there first for readability. I don't want to change the code convention just to comply with one case based on one specific compiler, especially if the change is less readable or introduce another issue:

  • IMK, it's a bad practice to start a variable with _ in C++ as it could be reserved to the implementation in a few cases. I would prefer another option to fix the warning.
  • The commenting of the parameter is debatable. On one hand, it indicates the parameter is not used, on another hand, if it's useless, why keeping it?

Why don't use pragma to disable warning when including tap.h?
Could you tell me which compiler and version do you use?

@kinow
Copy link
Owner

kinow commented Jul 16, 2016

ping @BruceBlank

@BruceBlank
Copy link
Author

Hi,

sorry for the delay: We use gcc version 4.8.5 (Ubuntu 4.8.5-1ubuntu1).

greetings
Bruce

@toch
Copy link
Collaborator

toch commented Jul 22, 2016

@BruceBlank What about using the following pragma to disable the warning on tap.h?

For instance

#pragma GCC diagnostic push 
#pragma GCC diagnostic ignored "-Wshadow"
#pragma GCC diagnostic ignored "-Wunused-variable"
#include <tap.h>
#pragma GCC diagnostic pop

not all diagnostic can be ignored, but I think your case should be covered. As the doc says:

Note that not all diagnostics are modifiable; at the moment only warnings (normally controlled by ‘-W...’) can be controlled, and not all of them. Use -fdiagnostics-show-option to determine which diagnostics are controllable and which option controls them.

@BruceBlank
Copy link
Author

@toch Thank you for your advice. I will try it.

@toch
Copy link
Collaborator

toch commented Jul 26, 2016

Keep me informed. if it works, it's a nice addition to our documentation I think.

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.

3 participants