RFR(XS): 8168490: Use the LL/ULL suffixes to define 64-bit integer literals on Windows

Tue Oct 25 14:57:40 UTC 2016

On Tue, Oct 25, 2016 at 8:09 AM, Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
> Hi Volker,
>
> so you didn't like my old SIZE_.. macros huh :)
>

I loved them :) but it was not so easy to keep them in the compiler
specifix _xlc file now that the CONST64/UCONST64 macros are defined in
the general globalDefinitions.hpp file.

> The changes are fine, and strictly speaking make the AIX port more correct,
> because before it was assumed all over the place that size_t is 64bit, and
> now we use the correct size_t typed constants. So should we ever do an AIX
> 32bit port this will help :)
>
> Nitpicks:
>
> - globals_aix.hpp: you can remove the UCONST64 macro on
> MaxExpectedDataSegmentSize.

I fixed that. Without UCONST64 it is only correct on 64-bit platforms
but it looks nicer and I doubt we'll ever do a 32bit AIX port :)

> - virtualspace.cpp: can you remove pls also remove the SIZE_xx definitions
> and usage around get_attach_addresses_for_disjoint_mode()?
>

I'd rather leave that for another clean-up because it is not really
related to this change.

Following the hopefully final webrev:

http://cr.openjdk.java.net/~simonis/webrevs/2016/8168490.v3/

Thanks,
Volker

> Thanks, Thomas
>
>
> On Mon, Oct 24, 2016 at 5:36 PM, Volker Simonis <volker.simonis at gmail.com>
> wrote:
>>
>> So here comes the new webrev:
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/2016/8168490.v2/
>>
>> It moves the definition of CONST64/UCONST64 and min_jlong/max_jlong
>> from the compiler-specific files to globalDefinitions.hpp.
>>
>> I also had to remove some AIX-specific constant definitions and
>> replace them by the more standard '<number>*[K,M,G]' notation.
>>
>> I did build and smoke tested this on Linux/amd64, MaxOS X, AIX,
>> Solaris and Windows.
>>
>> Thanks,
>> Volker
>>
>>
>> On Mon, Oct 24, 2016 at 2:51 PM, Volker Simonis
>> <volker.simonis at gmail.com> wrote:
>> > Hi Mikael,
>> >
>> > you're right - the OpenJDK now has the same definitions for
>> > CONST64/UCONST64 on all platforms. If you don't have other settings
>> > for you're closed ports I can certainly generalize the definition into
>> > globalDefinitions.hpp (it also works for HP-UX which is our last
>> > closed platform :)
>> >
>> > Notice that this would also require to move the definition of
>> > min_jlong/max_jlong from the compiler-specific files to
>> > globalDefinitions.hpp.
>> >
>> > I can do a new webrev with these additional changes.
>> >
>> > Regards,
>> > Volker
>> >
>> >
>> > On Mon, Oct 24, 2016 at 11:21 AM, Mikael Gerdin
>> > <mikael.gerdin at oracle.com> wrote:
>> >> Hi Volker,
>> >>
>> >>
>> >> On 2016-10-24 10:53, Volker Simonis wrote:
>> >>>
>> >>> On Mon, Oct 24, 2016 at 9:14 AM, David Holmes
>> >>> <david.holmes at oracle.com>
>> >>> wrote:
>> >>>>
>> >>>> On 24/10/2016 5:12 PM, Volker Simonis wrote:
>> >>>>>
>> >>>>>
>> >>>>> On Mon, Oct 24, 2016 at 2:43 AM, David Holmes
>> >>>>> <david.holmes at oracle.com>
>> >>>>> wrote:
>> >>>>>>
>> >>>>>>
>> >>>>>> Hi Volker,
>> >>>>>>
>> >>>>>> On 22/10/2016 1:35 AM, Volker Simonis wrote:
>> >>>>>>>
>> >>>>>>>
>> >>>>>>>
>> >>>>>>> Hi,
>> >>>>>>>
>> >>>>>>> can I please have a review and sponsor for the following trivial
>> >>>>>>> fix:
>> >>>>>>>
>> >>>>>>> http://cr.openjdk.java.net/~simonis/webrevs/2016/8168490/
>> >>>>>>> https://bugs.openjdk.java.net/browse/JDK-8168490
>> >>>>>>>
>> >>>>>>> Currently we use the i64/ui64 suffixes to declare 64-bit integer
>> >>>>>>> literals on Windows (see
>> >>>>>>> src/share/vm/utilities/globalDefinitions_visCPP.hpp).
>> >>>>>>>
>> >>>>>>> Unfortunately this suffix is not known to Eclipse so most of a
>> >>>>>>> hotspot
>> >>>>>>> files have errors in an Eclipse project (e.g. NULL is defined as
>> >>>>>>> '0i64' which gives a syntax error in Eclipse, but NULL is used in
>> >>>>>>> most
>> >>>>>>> hotspot of the source files).
>> >>>>>>>
>> >>>>>>> Fortunately VS supports the more standard conforming LL/ULL
>> >>>>>>> suffixes
>> >>>>>>> (see http://en.cppreference.com/w/cpp/language/integer_literal)
>> >>>>>>> since
>> >>>>>>> at least VS2010 (see
>> >>>>>>>
>> >>>>>>> https://msdn.microsoft.com/en-us/library/00a1awxf(v=vs.100).aspx).I
>> >>>>>>> therefore propose to change the suffix for integer literals from
>> >>>>>>> i64/ui64 to LL/ULL on Windows.
>> >>>>>>
>> >>>>>>
>> >>>>>>
>> >>>>>>
>> >>>>>> In the sense that this changes the VIS specific code to use a more
>> >>>>>> stand
>> >>>>>> conforming form this seems fine.
>> >>>>>>
>> >>>>>> But as I wrote in the bug report, should we even be using this file
>> >>>>>> with
>> >>>>>> non
>> >>>>>> Visual Studio compilers?
>> >>>>>>
>> >>>>>
>> >>>>> What I meant with "Eclipse doesn't understand" the suffix is that
>> >>>>> the
>> >>>>> Eclipse C++ parser doesn't understand it. Eclipse just parses all
>> >>>>> the
>> >>>>> C++ files which belong to a project in order to build up its code
>> >>>>> database for code navigation. As such it reads
>> >>>>> globalDefinitions_visCPP.hpp on Windows (if we configure a
>> >>>>> corresponding project) or  globalDefinitions_gcc.hpp on Linux,
>> >>>>> etc...
>> >>>>>
>> >>>>> This does not mean that we are using globalDefinitions_visCPP.hpp
>> >>>>> for
>> >>>>> building with another compiler. And we have no IDE-specific headers
>> >>>>> (this actually makes no sense since the IDEs should use the same
>> >>>>> settings which are used for the actual builds).
>> >>>>>
>> >>>>> Not sure about other IDEs, but Eclipse's C++ parser doesn't
>> >>>>> understand
>> >>>>> the i64/ui64 suffix and as there is a better, more
>> >>>>> standard-conforming
>> >>>>> way of declaring 64-bit integer literals on Windows I've just
>> >>>>> proposed
>> >>>>> the cleanup.
>> >>>>>
>> >>>>> Does this answer your question?
>> >>>>
>> >>>>
>> >>>>
>> >>>> Yes - thanks for clarifying.
>> >>>>
>> >>>
>> >>> Your welcome! Could you then please sponsor this change :)
>> >>
>> >>
>> >> Does this change make the definition of CONST64 / UCONST64 the same on
>> >> all
>> >> globalDefinitions_x variants?
>> >> If so can we move the definition to the common file instead of having
>> >> it in
>> >> the per-compiler files?
>> >>
>> >> Thanks
>> >> /Mikael
>> >>
>> >>
>> >>>
>> >>>>
>> >>>> David
>> >>>>
>> >>>>> Thank you and best egards,
>> >>>>> Volker
>> >>>>>
>> >>>>>
>> >>>>>> Thanks,
>> >>>>>> David
>> >>>>>>
>> >>>>>>
>> >>>>>>> Thank you and best regards,
>> >>>>>>> Volker
>> >>>>>>>
>> >>>>>>
>> >>>>
>> >>
>
>