On Mon, 12 Feb 2024 12:43:09 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 discovered by the > compiler), I thi... Nice change. I looked at the runtime files, and surprised there weren't more. Thank you. ------------- Marked as reviewed by coleenp (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17806#pullrequestreview-1875580015
