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

Add -Wmissing-declarations #30759

Merged
merged 5 commits into from
May 23, 2019

Conversation

jbytheway
Copy link
Contributor

@jbytheway jbytheway commented May 22, 2019

Summary

SUMMARY: Infrastructure "Compile with -Wmissing-declarations by default"

Purpose of change

This warning helps keep code tidy and comprehensible by ensuring functions local to a single file are marked static. This offers:

  • Clarity for devs.
  • Allows you to know when they are unused and can be deleted.
  • Helps the optimizer make better choices.
  • Smaller export tables in libraries.

Describe the solution

Enable -Wmissing-declarations and tidy up remaining failures related to that warning.

Add to Makefile and CMakeLists.txt

@KorGgenT KorGgenT added the Code: Build Issues regarding different builds and build environments label May 22, 2019
@jbytheway jbytheway changed the title Add -Wmissing-declarations [WIP] Add -Wmissing-declarations May 22, 2019
@jbytheway
Copy link
Contributor Author

Looks like I forgot to check the json formatter code...

@jbytheway jbytheway changed the title [WIP] Add -Wmissing-declarations Add -Wmissing-declarations May 22, 2019
@kevingranade
Copy link
Member

kevingranade commented May 23, 2019

Well there's a point against moving the win curses build out of the PR build list :/

src/wincurse.cpp:65:14: error: no previous declaration for 'std::__cxx11::wstring widen(const string&)' [-Werror=missing-declarations]
 std::wstring widen( const std::string &s )
              ^
src/wincurse.cpp: In function 'bool WinCreate()':
src/wincurse.cpp:79:6: error: no previous declaration for 'bool WinCreate()' [-Werror=missing-declarations]
 bool WinCreate()
      ^
src/wincurse.cpp: In function 'void WinDestroy()':
src/wincurse.cpp:131:6: error: no previous declaration for 'void WinDestroy()' [-Werror=missing-declarations]
 void WinDestroy()
      ^
src/wincurse.cpp: In function 'void create_backbuffer()':
src/wincurse.cpp:145:6: error: no previous declaration for 'void create_backbuffer()' [-Werror=missing-declarations]
 void create_backbuffer()
      ^
src/wincurse.cpp: In function 'void add_alt_code(char)':
src/wincurse.cpp:210:6: error: no previous declaration for 'void add_alt_code(char)' [-Werror=missing-declarations]
 void add_alt_code( char c )
      ^
src/wincurse.cpp: In function 'void CheckMessages()':
src/wincurse.cpp:537:6: error: no previous declaration for 'void CheckMessages()' [-Werror=missing-declarations]
 void CheckMessages()
      ^
src/wincurse.cpp: In function 'uint64_t GetPerfCount()':
src/wincurse.cpp:639:10: error: no previous declaration for 'uint64_t GetPerfCount()' [-Werror=missing-declarations]
 uint64_t GetPerfCount()
          ^
cc1plus: all warnings being treated as errors
Makefile:768: recipe for target 'objwin/wincurse.o' failed
make: *** [objwin/wincurse.o] Error 1
make: *** Waiting for unfinished jobs....

jbytheway added 3 commits May 23, 2019 09:49
This warning helps keep code tidy and comprehensible by ensuring
functions local to a single file are marked static, and get removed when
no longer used.

It also helps the optimizer reason about the code.
@jbytheway jbytheway force-pushed the missing_declarations branch from a48f773 to 28860a9 Compare May 23, 2019 08:49
@jbytheway
Copy link
Contributor Author

Looks promising. Gorgon complaint was just due to the Makefile change.

@kevingranade kevingranade merged commit 30f4e97 into CleverRaven:master May 23, 2019
@jbytheway jbytheway deleted the missing_declarations branch May 23, 2019 20:53
jbytheway added a commit to jbytheway/Cataclysm-DDA that referenced this pull request May 24, 2019
Merging CleverRaven#30759 broke the build by introducing a compile failure in
chkjson.  This should be built on Travis to allow detection of such
issues.

Previously chkjson was only built on Jenkins, but that doesn't happen
when the Makefile changes, so this problem went undetected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code: Build Issues regarding different builds and build environments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants