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

trac-3606: fix remaining msvc /W4 complaints (msvc-10.0 and up) #62

Merged
merged 2 commits into from
Jan 18, 2018

Conversation

jeking3
Copy link
Contributor

@jeking3 jeking3 commented Dec 27, 2017

MSVC fixes: https://svn.boost.org/trac10/ticket/3606
MSVC fixes added a top level jamfile which turned on some additional warnings.

GCC/CLANG fixes: https://svn.boost.org/trac10/ticket/9882 (-Wshadow, -Wimplicit-fallthrough)

@eldiener
Copy link
Contributor

What is the reason for the changes to variable names in this PR ?

@jeking3
Copy link
Contributor Author

jeking3 commented Jan 1, 2018

Variable shadowing warnings. I'll fix the .gitignore.

@eldiener
Copy link
Contributor

eldiener commented Jan 1, 2018

I do not think that changing the names of variables to avoid shadow warnings should ever be a programmable way of doing things. I would much rather have the warning turned off for the particular compiler, if we decide to do anything. If we start doing that there will be no end to changing library code just because some compiler gives a warning because the same variable name is used in different nested scopes. This is perfectly legal c++ and if no one needs to ever write code just to avoid such warnings.

@mclow
Copy link
Contributor

mclow commented Jan 1, 2018

I disagree; I believe that shadowing warnings are valuable; they let the programmer know that he might not be referencing the variable that he expected to.

Unlike the stupid "truncation - may cause loss of data" warning that MSVC has (which can DIAF), I think these are worth fixing.

@eldiener
Copy link
Contributor

eldiener commented Jan 1, 2018

In what type of situations do these shadow warnings occur for which vc++ gives a warning ?

@jeking3
Copy link
Contributor Author

jeking3 commented Jan 3, 2018

Environmental issue; rebuilding with an empty rebase.

@jeking3
Copy link
Contributor Author

jeking3 commented Jan 12, 2018

I've no way of telling if these are existing build issues or new ones... given the specific nature of this pull request I would say these platforms were already broken?

@eldiener
Copy link
Contributor

I am able to run the date_time tests against the current developer branch using mingw-64/gcc-6.3 x86_64 but with rev2 rather than rev1. This does not include this current PR. The test runs without errors for both c03 and c11 modes.

I am able to run the date_time tests against the current developer branch using mingw-64/gcc-5.3 i686 rev 0. This does not include the current PR. The test runs with a single already well-known error in both c03 and c11 modes.

It would be helpful for you to setup a local mingw-64 environment for running tests to match the Appveyor mingw tests. I would be glad to help you to do this if you had problems. That way you could see why the Appveyor tests have so many failures with your current PR fix. It may have to do with your modifications or it just may be some anomaly of the Appveyor testing environment.

I have never run local cygwin tests but maybe someone else has.

@jeking3
Copy link
Contributor Author

jeking3 commented Jan 12, 2018

I have local build environments for all of these; I will look into it.
I'm also going to refresh the other PRs to see if they fail the same way.
Also, what's with the day-long delay on the Travis builds?

@jeking3
Copy link
Contributor Author

jeking3 commented Jan 17, 2018

All right, everything passed. Let me know if you want any more changes here.

@jeking3 jeking3 merged commit 313c1a3 into boostorg:develop Jan 18, 2018
@jeking3 jeking3 deleted the trac-3606 branch January 18, 2018 03:10
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.

None yet

3 participants