6348631 - request for review (updated)

[David Holmes]

Ivan Krylov said the following on 12/02/10 08:33:
> David,
> On 02.12.2010 1:15, David Holmes wrote:
>> Hmmm
>>
>> coleen phillimore said the following on 12/01/10 04:28:
>>> This looks good to me.  I have some concerns with the inlined socket 
>>> functions in:
>>>
>>> http://cr.openjdk.java.net/~ikrylov/6348631.v3/src/os/linux/vm/os_linux.inline.hpp.cdiff.html 
>>>
>>>
>>> as it causes the .hpp file to import linux system specific hpp files 
>>> which might cause namespace issues down the line.  I'd rather these 
>>> sorts of includes be in a .cpp file.  I don't know if they were 
>>> inlined for performance reasons though, but it seems unlikely to have 
>>> any benefit.
>>
>> When I was taught C programming I was taught that each file should be 
>> self-contained and include all headers that it needed. That way if I 
>> include a header to get the definitions for the "interface" to some 
>> library, I don't have to know what all of its dependencies are - I 
>> just include one header and call the library functions. I don't have 
>> to discover all its dependencies by trial and error (nor does the 
>> provider of the library have to document them all).
>>
>> Arguably for our xxx.inline.hpp files it is a different situation 
>> because they should only be included by our own xxx.cpp file. But as a 
>> general rule I would not advocate removing all system includes from 
>> all hotspot headers.
> 
> These files include os_linux.inline.hpp explicitly now.

Hmmm that seems "wrong". I would expect the cpp files to include 
os_linux.hpp and then _that_ file would include os_linux.inline.hpp

But in a way this reinforces my point. If os_linux.inline.cpp includes 
say 6 system headers, we don't want to have to include those same 6 
headers in every single file that includes os_linux.inline.hpp!

I guess now that we've made the include dependencies explicit we're 
going to realize what an absolute mess they are in places. :(

David

> ./share/vm/classfile/classLoader.cpp
> ./share/vm/runtime/synchronizer.cpp
> ./share/vm/runtime/handles.cpp
> ./share/vm/runtime/thread.cpp
> ./share/vm/runtime/mutexLocker.hpp
> ./share/vm/runtime/timer.cpp
> ./share/vm/runtime/arguments.cpp
> ./share/vm/runtime/virtualspace.cpp
> ./share/vm/runtime/task.cpp
> ./share/vm/runtime/threadLocalStorage.cpp
> ./share/vm/runtime/atomic.cpp
> ./share/vm/runtime/os.cpp
> ./share/vm/runtime/objectMonitor.cpp
> ./share/vm/memory/space.hpp
> ./share/vm/memory/gcLocker.hpp
> ./share/vm/memory/allocation.cpp
> ./share/vm/prims/nativeLookup.cpp
> ./share/vm/prims/jni.cpp
> ./share/vm/compiler/disassembler.hpp
> ./share/vm/code/stubs.hpp
> ./share/vm/utilities/bitMap.cpp
> ./share/vm/utilities/accessFlags.cpp
> ./share/vm/utilities/histogram.hpp
> ./share/vm/utilities/ostream.cpp
> ./share/vm/utilities/debug.cpp
> ./share/vm/gc_implementation/parallelScavenge/parMarkBitMap.cpp
> ./share/vm/gc_implementation/parallelScavenge/psVirtualspace.cpp
> ./share/vm/gc_implementation/concurrentMarkSweep/cmsAdaptiveSizePolicy.cpp
> ./os/linux/vm/perfMemory_linux.cpp
> ./os/linux/vm/mutex_linux.inline.hpp
> ./cpu/sparc/vm/vm_version_sparc.cpp
> ./cpu/zero/vm/vm_version_zero.cpp
> ./cpu/x86/vm/vm_version_x86.cpp
> 
> Even worse: when gcc generates a precompiled header it wants to see 
> definitions of those system structures and functions.That is before any 
> cpp file is being parsed.
> 
> Thanks,
> Ivan
>