On Tue, 13 Feb 2024 11:05:30 GMT, Magnus Ihse Bursie <i...@openjdk.org> wrote:
>> There are several places in hotspot where an internal function should have >> been declared static, but isn't. >> >> These were discovered by trying to use the gcc option >> `-Wmissing-declarations` and the corresponding clang option >> `-Wmissing-prototypes`. These warnings check that a function either: >> a) is declared static, or >> b) has a declaration before its definition. >> >> The rationale of this is that functions are either internal to a compilation >> unit, or exported to be linked by some other compilation unit. In the former >> case, it should be marked static. In the latter case, it should be declared >> in a header file, which should be included by the implementation as well. If >> there is a discrepancy between the exported prototype and the implemented >> prototype, this will be discovered during compilation (instead of as a >> runtime error). Additionally, marking internal methods as static allows the >> compiler to make better optimization, like inlining. >> >> This seem to be to be a sane assumption, and I think Hotspot (and the entire >> JDK) would increase code quality by turning on these warnings. The absolute >> majority of the code already adheres to these rules, but there are still >> some places that needs to be fixed. >> >> This is the first part of addressing these issues, where all places that are >> trivially missing static are fixed. >> >> I have discovered these by running with the warnings mentioned above turned >> on. I have filtered out those places were an export was obviously missing. >> The remaining warnings I have manually inspected. About 1/4 of them were >> *_init() functions (which are directly declared in `init.cpp`) and another >> 1/4 were documented as "use in debugger"; these I have not touched. I also >> ignored functions with names suggesting it might be used in the debugger, >> even if not documented as such, or any places that even seemed remotely >> non-trivial. Finally I also reverted a few changes after it turned out that >> gcc complained about unused functions. These places are actually dead code, >> but it is not clear if they should be removed or if there is actually a call >> missing somewhere (I believe it is a mix of both). In any case, I did not >> want any such complexities in this PR. >> >> When the functions where marked static, gcc started complaining if they were >> not used, since it consider it dead code. To address this, I had to add or >> fix some `#ifdef`s. Since this code were not actually used unless these >> criteria were fulfilled before either (it was just not disc... > > Magnus Ihse Bursie has updated the pull request incrementally with two > additional commits since the last revision: > > - Revert spurious changes in non-modified file > - Fix indentation issues Looks good. ------------- Marked as reviewed by kbarrett (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17806#pullrequestreview-1880692930
