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 various issues with MinGW build #9

Merged
merged 5 commits into from Dec 21, 2020
Merged

Conversation

LordAro
Copy link
Member

@LordAro LordAro commented Oct 31, 2020

Most compilation warnings fixed. (Almost) no changes in behaviour - added an obviously missing break. Other than that, I've tried to keep to the lovely codestyle.

Two remaining warnings that I haven't quite figured out, as they would require some thought on my part:

[CPP] objs/grfcomm.o
In file included from src/grfcomm.cpp:10:
In function 'char* strncpy(char*, const char*, size_t)',
    inlined from 'char* spritefilename(const char*, const char*, const char*, int, const char*, int)' at src/grfcomm.cpp:216:9:
C:/msys64/mingw64/x86_64-w64-mingw32/include/string.h:240:33: warning: 'char* __builtin_strncpy(char*, const char*, long long unsigned int)' specified bound 1024 equals destination size [-Wstringop-truncation]
  240 |   return __builtin___strncpy_chk(__dst, __src, __n, __mingw_bos(__dst, 1));
      |          ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In function 'char* strncpy(char*, const char*, size_t)',
    inlined from 'int getspritefilename(char*, const char*, char*, const char*, long int)' at src/grfcomm.cpp:216:9,
    inlined from 'char* spritefilename(const char*, const char*, const char*, int, const char*, int)' at src/grfcomm.cpp:114:19:
C:/msys64/mingw64/x86_64-w64-mingw32/include/string.h:240:33: warning: 'char* __builtin_strncpy(char*, const char*, long long unsigned int)' output may be truncated copying 2 bytes from a string of length 2 [-Wstringop-truncation]
  240 |   return __builtin___strncpy_chk(__dst, __src, __n, __mingw_bos(__dst, 1));
      |          ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

(Because the message isn't clear, they correspond to

safestrncpy(directory, reldirectory, MAXDIR);
and
safestrncpy(bdrive, sdrive, MAXDRIVE);
respectively)

src/data.cpp Outdated Show resolved Hide resolved
src/nforenum.cpp Outdated Show resolved Hide resolved
@glx22
Copy link
Contributor

glx22 commented Nov 1, 2020

Hmm I have a cmake branch based on @PeterN cmake branch. I should probably PR it.

@FLHerne
Copy link
Contributor

FLHerne commented Nov 1, 2020

LGTM now from reading and Linux compile, but I don't have a Windows build env.

@glx22
Copy link
Contributor

glx22 commented Nov 1, 2020

Oh for windows it's easy, currently MSVC is not supported, only MinGW.

@glx22
Copy link
Contributor

glx22 commented Nov 1, 2020

I checked MinGW (both 32bit and 64bit) and it works.
BTW I don't have the warnings in the cmake branch.

@glx22
Copy link
Contributor

glx22 commented Nov 4, 2020

OK, I restored compile flags from Makefile in cmake. MinGW warnings are now visible, I then cherry picked your latest commit and all warnings are gone in my branch. But I still don't have grfcomm warnings.

@glx22
Copy link
Contributor

glx22 commented Dec 15, 2020

Ok, grfcomm warnings are for release builds.

Copy link
Contributor

@glx22 glx22 left a comment

Choose a reason for hiding this comment

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

Works for me

src/data.cpp Outdated
@@ -1148,6 +1148,8 @@ FILE*_myfopen(files file, bool write){
exit(EDATA);
}
} else
#else
(void)write;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use tabs for indentation ;) (noticed it with glx22#2)

Copy link
Contributor

@glx22 glx22 left a comment

Choose a reason for hiding this comment

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

Should be ok now

@glx22 glx22 merged commit 5a00dc3 into OpenTTD:master Dec 21, 2020
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