search

carbon-language / carbon-lang

Carbon Language's main repository: documents, design, implementation, and related tools. (NOTE: Carbon Language is experimental; see README)
https://github.com/carbon-language/carbon-lang/blob/trunk/README.md
Other
32.31k stars 1.48k forks source link

Add `-Wmissing-prototypes` and fix issues it finds. #4019

Closed chandlerc closed 4 weeks ago

chandlerc commented 1 month ago

Most of these are places where we failed to include a header file and simply never got an error about this. The fix is to include the header file.

Most other cases are functions that should have been marked static but were not. Finding all of these was a main motivation for me enabling the warning despite how much work it is.

One complicating factor was that we weren't including the handle.h for all the state-based handler functions. While this isn't a tiny amount of code, it is just declarations and doesn't add any extra dependencies. It also lets us have the checking for which functions need to be static and which don't. For the parse library I had to add the handle.h header as well, I tried to match the design of it in check.

I have also had to work around a bug in the warning, but given the value it seems to be providing, that seems reasonable. I've filed the bug upstream: https://github.com/llvm/llvm-project/issues/94138

I also had to use some hacks to work around limitations of Bazel rules that wrap cc_library rules and don't expose copts. I filed a bug for cc_proto_library specifically: ~https://github.com/bazelbuild/bazel/issues/22610~ https://github.com/bazelbuild/bazel/issues/4446

chandlerc commented 4 weeks ago

PTAL and lemme know if this is good, or a different approach. Happy to also look into class methods or something else if more appropriate.