RFR (also tedious) 8216167: Update include guards to reflect correct directories

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Thu Jan 10 15:20:14 UTC 2019



On 1/10/19 9:56 AM, Erik Österlund wrote:
> Hi Coleen,
>
> share/gc/shared/barrierSet.inline.hpp Was added recently, needs an 
> update.

Fixed by hand.

> share/prims/wbtestmethods/parserTests.hpp Still has VM prefix in 
> include guard, and lacks a whitespace in the #endif part.

I already fixed this.   I updated 02 with the new version so reload...
>
> share/gc/parallel/psGenerationCounters.hpp Has a leading newline, 
> which I think should be removed if we are fixing incorrect trailing 
> newlines.

Great, yes, I removed leading blank line.
> share/opto/adlcVMDeps.hpp Seems to do something wrong now. This file 
> is included both by libjvm.so and the adlc parser binary. It 
> conditionally includes memory/allocation.hpp if it is built in 
> libjvm.so, which is figured out by checking if the adlc arena 
> allocation header has already been included or not with the following 
> hack:
>
> #ifndef SHARE_VM_ADLC_ARENA_HPP
> #include "memory/allocation.hpp"
> #endif
>
> You changed SHARE_VM_ADLC_ARENA_HPP here to SHARE_OPTO_ADLCVMDEPS_HPP 
> in the patch you posted, probably because your script thought it was 
> an include guard. But it is not. It's an ugly hack.
>
> So with the changes you proposed, the condition would always be true, 
> which is not intended. It should check for SHARE_ADLC_ARENA_HPP 
> instead now.

This file.  I fixed it with the #pragma once change, then forgot which 
one it was.   This file doesn't need to include allocation if it doesn't 
use AllStatic so I changed it like this:

diff --git a/src/hotspot/share/opto/adlcVMDeps.hpp 
b/src/hotspot/share/opto/adlcVMDeps.hpp
--- a/src/hotspot/share/opto/adlcVMDeps.hpp
+++ b/src/hotspot/share/opto/adlcVMDeps.hpp
@@ -22,20 +22,18 @@
   *
   */

-#ifndef SHARE_VM_OPTO_ADLCVMDEPS_HPP
-#define SHARE_VM_OPTO_ADLCVMDEPS_HPP
+#ifndef SHARE_OPTO_ADLCVMDEPS_HPP
+#define SHARE_OPTO_ADLCVMDEPS_HPP
+

  // adlcVMDeps.hpp is used by both adlc and vm builds.
-// Only include allocation.hpp when we're not building adlc.
-#ifndef SHARE_VM_ADLC_ARENA_HPP
-#include "memory/allocation.hpp"
-#endif
+// Don't inherit from AllStatic to avoid including memory/allocation.hpp.

  // Declare commonly known constant and data structures between the
  // ADLC and the VM
  //

-class AdlcVMDeps : public AllStatic {
+class AdlcVMDeps {   // AllStatic
   public:
    // Mirror of TypeFunc types
    enum { Control, I_O, Memory, FramePtr, ReturnAdr, Parms };
@@ -52,4 +50,4 @@
    static const char* none_reloc_type() { return "relocInfo::none"; }
  };

-#endif // SHARE_VM_OPTO_ADLCVMDEPS_HPP
+#endif // SHARE_OPTO_ADLCVMDEPS_HPP


>
> Otherwise this looks... I don't know... okay. But #pragma once would 
> have been so much better. :c

Yeah.  I know.  Thank you for looking through all of this.

Coleen
>
> Thanks,
> /Erik
>
> On 2019-01-10 13:34, coleen.phillimore at oracle.com wrote:
>>
>> No takers?  This fixes the include guard to match the file name in 
>> 1540 header files.
>>
>> If you add a header file, please use the file name after src/hotspot 
>> as the include guard name from now on (exclude the VM that used to be 
>> there).
>>
>> http://cr.openjdk.java.net/~coleenp/8216167.01.diff.02
>>
>> Thanks,
>> Coleen
>>
>> On 1/4/19 6:19 PM, coleen.phillimore at oracle.com wrote:
>>>
>>>
>>> On 1/4/19 4:52 PM, Kim Barrett wrote:
>>>>> On Jan 4, 2019, at 10:36 AM, coleen.phillimore at oracle.com wrote:
>>>>>
>>>>> Summary: Use script and some manual fixup to fix directores names 
>>>>> in include guards.
>>>>>
>>>>> Makes include guards match the current directory rooted at 
>>>>> src/hotspot (removes VM_ in most cases).
>>>>>
>>>>> This should be low risk.  Tested with mach5 tier1 and tier2.
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8216167
>>>>> http://cr.openjdk.java.net/~coleenp/8216167.01.diff
>>>>>
>>>>> I didn't generate a webrev as a space concern for 
>>>>> cr.openjdk.java.net and nobody should click on it.  Script is 
>>>>> posted in bug.  Will update and check copyright headers with hg 
>>>>> commit.
>>>>>
>>>>> Thanks,
>>>>> Coleen
>>>> There are incorrect changes in 
>>>> src/hotspot/cpu/arm/globalDefinitions_arm.hpp
>>>
>>> Thank you for finding this.
>>>> The script is not being careful to *only* modify #include guards.  
>>>> I didn’t look for other similar problems.
>>>> (This is an example of why I suggested rolling your own script 
>>>> might not actually be easier than using
>>>> the guardonce utilities.)
>>>>
>>> I looked through again and didn't see any other problems.  I'm not 
>>> planning on productizing my script, which admittedly is too simple 
>>> for the general job.  But I still found it useful and entertaining 
>>> (to me) for this particular task.
>>>
>>> http://cr.openjdk.java.net/~coleenp/8216167.01.diff.02
>>>
>>> Thanks,
>>> Coleen
>>
>



More information about the hotspot-dev mailing list