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

Tue Oct 25 15:31:04 UTC 2016

Looks fine!

On Tuesday, 25 October 2016, Volker Simonis <volker.simonis at gmail.com>
wrote:

> On Tue, Oct 25, 2016 at 8:09 AM, Thomas Stüfe <thomas.stuefe at gmail.com
> <javascript:;>> 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 <javascript:;>>
> > 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 <javascript:;>> 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 <javascript:;>> 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 <javascript:;>>
> >> >>> 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 <javascript:;>>
> >> >>>>> 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
> >> >>>>>>>
> >> >>>>>>
> >> >>>>
> >> >>
> >
> >
>