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.

Sign up for GitHub

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

Jump to bottom

VC14 Cleanup #894

Merged
merged 8 commits into from
Dec 4, 2016
Merged

VC14 Cleanup #894

merged 8 commits into from
Dec 4, 2016

Conversation

GregoryLundberg
Copy link
Contributor

These commits should get us to 0 errors / 0 warnings for both Release and Debug builds for Visual Studio 2015.

In general there were three classes of error:

* A local name hiding another. Renamed the local name

* The poorly-written logging macro causing a duplicated name to hide an earlier name. Removed the duplicate macro. This eliminated some inner log scopes. A better fix would be to completely replace the lame macro with a better one; but that is too much work for today.

* Bugs in spirit_po and certain Boost libraries. These were names-hides-name errors. Some could have been fixed by renaming some Wesnoth global variables; some are true bugs in the libraries. Since these are libraries, they messages were suppressed rather than correcting the library.

@Vultraz
Copy link
Member

Vultraz commented Dec 3, 2016

Don't merge this yet, I'd like to check why I declared two data/item vars in mp_staging.

@Vultraz
Copy link
Member

Vultraz commented Dec 3, 2016

See 82a8797

@Vultraz
Copy link
Member

Vultraz commented Dec 3, 2016

Wait, 8722757. Sorry >_>

CelticMinstrel
CelticMinstrel reviewed Dec 3, 2016
View reviewed changes
Copy link
Member

@CelticMinstrel CelticMinstrel left a comment

Choose a reason for hiding this comment

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

Largely looks good. I think I'd prefer if the "pragma-include-unpragma" blocks were set apart by blank lines from the rest of the includes, though. Also, I'm not exactly keen on removing log scopes just to fix some variable hiding warnings.

src/gettext_boost.cpp
@@ -153,18 +153,18 @@ namespace
throw game::error(err.str());
}

const char* get(int domain_id, const char* ctx, const char* id) const override
const char* get(int domain_id, const char* ctx, const char* id_param) const override
Copy link
Member

Choose a reason for hiding this comment

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

What about msg_id? Seems more descriptive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

src/gui/widgets/window.cpp
@@ -1243,8 +1243,8 @@ void swap_grid(grid* g,
parent_grid = find_widget<grid>(content_grid, id, true, false);
assert(parent_grid);
}
if(grid* g = dynamic_cast<grid*>(parent_grid->parent())) {
widget = g->swap_child(id, widget, false);
if(grid* g_in_if = dynamic_cast<grid*>(parent_grid->parent())) {
Copy link
Member

Choose a reason for hiding this comment

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

This is a really terrible name. I think I'd prefer renaming the parameter instead in this case. Not entirely sure what's a good name though. Maybe the doxy-comment in the header can give a good idea.

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 like terrible names. I don't like to think when fixing dumb mistakes. That can be done later. The really dumb names should be read as "I'm disgusted that I even have to do look for this mistake,"

src/gettext_boost.cpp
@@ -11,6 +11,9 @@

See the COPYING file for more details.
*/
#if defined(_MSC_VER)
#pragma warning(disable: 4714)
Copy link
Member

Choose a reason for hiding this comment

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

Wait, why did you move this up here instead of just adding another warning to the other pragma?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was originally there. I moved it to make it apparent it's fully file-scoped. Note no pop/push either.
The warning occurs during link-time code generation.

Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure it was supposed to be just for spirit_po though, which is why it was there; so the mistake is probably forgetting the push/pop.

GregoryLundberg
GregoryLundberg commented Dec 3, 2016
View reviewed changes
src/gettext_boost.cpp
@@ -30,7 +33,7 @@
#pragma GCC diagnostic ignored "-Wmissing-field-initializers"
#elif defined(_MSC_VER)
#pragma warning(push)
#pragma warning(disable: 4714)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

4714 was originally down here
4459 was needed so I added it
then I noticed 4714 was not working here, so I moved it

@GregoryLundberg
Copy link
Contributor Author

  1. dropped commit per @Vultraz changing
  2. renamed variables to better names
  3. put logging scopes back in, hand-expanded lame macro to avoid errors

This should address all comments.

@CelticMinstrel
Copy link
Member

Well, I don't like hand-expanding the macro, but I guess it's better than removing the log scopes altogether.

@GregoryLundberg
Copy link
Contributor Author

Actually, I don't like anything about the macro and want to re-write it but with everything going on here, I am trying to do small things and not think too hard.

Fixing the macros should be a separate subject PR. So consider the hand-expansion as hand-waving to point out it needs doing.

@CelticMinstrel CelticMinstrel merged commit 707c9d7 into wesnoth:master Dec 4, 2016
@GregoryLundberg GregoryLundberg deleted the GL_VC14_cleanup branch December 4, 2016 04:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CelticMinstrel CelticMinstrel CelticMinstrel left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@GregoryLundberg @Vultraz @CelticMinstrel