[m-rev.] for review: delete the hl grade component

[Zoltan Somogyi]

On Sat, 11 Apr 2020 14:14:52 +1000 (AEST), Julien Fischer <jfischer at opturion.com> wrote:
> On Sat, 11 Apr 2020, Zoltan Somogyi wrote:
> > I also have a question. This diff deletes every occurrence of
> > the C macro MR_HIGHLEVEL_DATA, since it is now unnecessary,
> > but a bunch of occurrences of that string remain in C# code,
> > almost all in #ifdefs in library/rtti_implementation.m. The
> > #else parts of those #ifdefs seem to me to be relics of the
> > .NET backend, when it tried to access low-level-data RTTI.
> > Since compile_target_code.m *always* defined MR_HIGHLEVEL_DATA
> > for C#, I think the non-MR_HIGHLEVEL_DATA sides of those
> > #ifdefs are dead code, but I am not 100% sure. Does anyone
> > have any reason to believe that these apparently dead pieces
> > of code are in fact needed? And if not, is there any reason why
> > we need to continue defining MR_HIGHLEVEL_DATA for C#?
> 
> They are dead code; they were used for the ilc grade -- they should
> have been removed when it was removed.

I have removed it.

> > compiler/compute_grade.m:
> >     Delete references to the hl grade component, and conform
> >     to the deletion of the --high-level-data option.
> > 
> > compiler/handle_options.m:
> 
> Did you leave something out here?

I moved handle_options.m to the block of filenames
that conform to the changes above.

> > diff --git a/NEWS b/NEWS
> > index 6c1e3e523..b0e4560ec 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -203,6 +203,10 @@ Changes to the Mercury compiler
> >    that the `--trail-segments` option now has no effect, and it is therefore
> >    deprecated.
> > 
> > +* `--high-level-data`
> > +
> > +  We have deleted the --high-level-data option.
> 
> Quote --high-level-data there.

Done.
 
>      We have deleted the `--high-level-data` option. Its effects are now
>      implied by the target language.

I don't think that is perfect either, but it is better than what I had, so it is added.

> > @@ -2881,14 +2871,7 @@ create_csharp_exe_or_lib(Globals, ErrorStream, LinkTargetType, MainModuleName,
> >      % NOTE: we use the -option style options in preference to the /option
> >      % style in order to avoid problems with POSIX style shells.
> >      globals.lookup_string_option(Globals, csharp_compiler, CSharpCompiler),
> > -    globals.lookup_bool_option(Globals, highlevel_data, HighLevelData),
> > -    (
> > -        HighLevelData = yes,
> > -        HighLevelDataOpt = "-define:MR_HIGHLEVEL_DATA"
> > -    ;
> > -        HighLevelData = no,
> > -        HighLevelDataOpt = ""
> > -    ),
> > +    HighLevelDataOpt = "-define:MR_HIGHLEVEL_DATA",
> >      globals.lookup_bool_option(Globals, target_debug, Debug),
> >      (
> >          Debug = yes,
> 
> As noted elsewhere in this thread, defining MR_HIGHLEVEL_DATA here should
> not be necessary.  (The non-MR_HIGHLEVEL_DATA C# code paths should be deleted,
> they were for the old ilc grade.)

Both done.

> >  ml_generate_const_structs(ModuleInfo, Target, ConstStructMap, !GlobalData) :-
> > -    module_info_get_globals(ModuleInfo, Globals),
> > -    globals.lookup_bool_option(Globals, highlevel_data, HighLevelData),
> > +    (
> > +        Target = ml_target_c,
> > +        HighLevelData = no
> > +    ;
> > +        ( Target = ml_target_java
> > +        ; Target = ml_target_csharp
> > +        ),
> > +        HighLevelData = yes
> > +    ),
> 
> This chunk of code is repeated in a few spots, perhaps it should be a separate
> predicate in ml_util.m?

Done.

> > diff --git a/scripts/parse_ml_options.sh-subr.in b/scripts/parse_ml_options.sh-subr.in
> > index 4c8e8d3ca..bc13e1d16 100644
> > --- a/scripts/parse_ml_options.sh-subr.in
> > +++ b/scripts/parse_ml_options.sh-subr.in
> > @@ -1,4 +1,6 @@
> >  #---------------------------------------------------------------------------#
> > +# vim: ts=4 sw=4 expandtab ft=sh
> > +#---------------------------------------------------------------------------#
> >  # Copyright (C) 2001-2007, 2011 The University of Melbourne.
> >  # This file may only be copied under the terms of the GNU General
> >  # Public License - see the file COPYING in the Mercury distribution.
> > @@ -18,7 +20,7 @@
> >
> >  # initialize common options
> >  mercury_config_dir=${MERCURY_STDLIB_DIR- at CONFIG_LIBDIR@}
> > -mercury_config_dir=${MERCURY_CONFIG_DIR=$mercury_config_dir}
> > +mercury_config_dir=${MERCURY_CONFIG_DIR=${mercury_config_dir}}
> >  mercury_stdlib_dir=${MERCURY_STDLIB_DIR=@LIBDIR@}
> >
> >  # initialize ml options
> > @@ -27,7 +29,7 @@ allow_undef=false
> >  trace=false
> >  ssdb=false
> >  readline=true
> > -case $FULLARCH in
> > +case ${FULLARCH} in
> >  	*-win95|*-winnt|*-win32|*-cygwin32|*-cygwin|*mingw*)
> 
> Not that it is relevant to this diff, but I'm pretty sure some
> of those alternatives (*-win95)  can go now.

I will leave that to you, but I don't think their presence is a huge
burden, and someone may still be using it :-(

> The diff looks fine otherwise.

Thanks for the review.

Zoltan.