[m-rev.] systematic problem with tests/valid* for C#

Julien Fischer jfischer at opturion.com
Wed Oct 4 15:50:54 AEDT 2023


On Tue, 3 Oct 2023, Zoltan Somogyi wrote:

> On 2023-10-02 19:24 +11:00 AEDT, "Julien Fischer" <jfischer at opturion.com> wrote:
>>> We have long had .all_cs as a synonym for .cs for this exact reason
>>> (just as we have .all_os as a synonym for .os), but stopped short of
>>> switching the meaning of a target with a .cs suffix to mean a C# source file.
>>>
>>> Would anyone object to making that step now?
>>
>> No.
>
> The attached diff does this. It bootchecks in C grades with and without --intermod-opt.
> It bootchecks in Java grade with the same number of test failures as before,
> and it bootchecks in C# with a reduced number of test fails, because the diff
> fixes the old fails caused by the mismatch in the meaning of target cs, as intended.
>
>>> Specifically, I propose that both mmake and mmc --make should have the
>>> rule that ONLY the suffix  ".all_Xs" means "all .X files".
>>
>> That's fine.
>
> I made a start on this, but found a problem that prevented me from going there
> immediately. The problem is explained in the third point added to NEWS.md.
>
> The diff is for review by anyone. The changes in it should be reflected by changes
> in the user guide, but I would like to see your review comments for NEWS.md
> before I make those changes.
>
> This is a sort-of unusual diff to review, because the main possible source of error in it
> is not the presence of some change in the attached diff, but the *absence* of a change
> in some file that would be needed to conform the changes that *are* in the diff.
> I would like reviewers to tell me if they know of any places that need such an update.
>
> For the change to NEWS.md, I am seeking feedback both about its wording, and
> the use of markup. For example, is there some standard way to communicate
> the meta-syntactic nature of `name` and `program`  in the newly added text?
> And I would also like to know whether we should talk about C#-related mmake vars,
> given that we don't support use of mmake for C#.

We also don't support mmake with Java. The issue here is that

1. Some mmake support for Java dates back to the original addition of the
    Java backend, when mmc --make wasn't really a thing.
2. Some mmake support for C# is (probably) left over from the old .NET
    backend.
3. Some mmake support for C# is (probably) just the result of
    copy-and-pasting when the present C# backend was added.
4. Some mmake support for both languages may be necessary for mmake's
    --use-mmc-make option to work.

We need to maintain support for (4), but everything else could go.
Unfortunately, that would mean working out which of the categories
each piece of C# or Java support in mmake falls into.

> Get "make cs"/"mmc --make x.cs" build a C# file ...

  *to* build

> 
> ... instead of building a bunch of .c files.
> 
> Our tradition of adding an "s" at the end of a suffix to mean "all of the
> files with the original suffix" had a problem when we added C# as a target
> language. Until then, just as "os" stood for ".o files" when it occurred
> as either a mmake target, mmc --make target, or mmake variable name component.
> "cs" likewise stood for ".c files", but was now also needed to mean ".cs file".
> We coped by keeping "cs" meaning ".c files", and adding "csharp" as a target
> name synonym to mean ".cs file".
> 
> This diff keeps that synonym, but it changes
> 
> - the name needed to refer to ".c files" from "cs" to "all_cs"
> - the name needed to refer to ".o files" from "os" to "all_os"
> - the name needed to refer to ".pic_o files" from "pic_os" to "all_pic_os"
> - the name needed to refer to ".cs files" from "css" to "all_css"
> - the name needed to refer to ".java files" from "javas" to "all_javas"
> - the name needed to refer to ".opt files" from "opts" to "all_opts"
> - the name needed to refer to ".trans_opt files"
>         from "trans_opts" to "all_trans_opts"
> 
> It would be nice if we could apply this same change to all other similar
> target names and mmake variable name suffixes, such as "ints" and "int3s",
> but so of those names are already in use to mean semantically different

s/so/some/

> things. All of the names above that used to have the form "<ext>s" and
> now have the form "all_<ext>s" stood for all the files with extension
> ".<ext>" that are prerequites for building a linked target, i.e. an executable

s/prerequites/prerequisites/

> or a library. But the mmake variable name suffixes ".all_mihs", ".all_mhs"
> and ".all_int0s" each stand for something subtly different: the names of
> files that *may or may not exist", but which, if they do exist, should be
> deleted by a clean or realclean target.

...

> scripts/Mmake.rules:
> scripts/Mmake.vars.in:

What's the change here?


> browser/Mmakefile:
> compiler/Mmakefile:
> deep_profiler/Mmakefile:
> grade_lib/Mmakefile:
> library/Mmakefile:
> mdbcomp/Mmakefile:
> mfilterjavac/Mmakefile:
> profiler/Mmakefile:
> slice/Mmakefile:
> ssdb/Mmakefile:
>     Rename os to all_os, cs to all_cs, css to all_css, javas to all_javas,
>     and opts to all_opts. (There were no occurrences of trans_opts to rename.)
>
>     Replace [s as sh command names in actions.
> 
> tools/bootcheck:
> diff --git a/NEWS.md b/NEWS.md
> index 1726dd440..66942f031 100644
> --- a/NEWS.md
> +++ b/NEWS.md
> @@ -58,6 +58,79 @@ Changes that may break compatibility
>
>  * We have dropped support for the x86 (32-bit) version of Cygwin.
> 
> +* We have changed the meaning of `mmc --make name.cs`.
> +
> +  We have long had two conventions for the names of mmc --make targets
> +  for building all the files of a particular kind needed by a program.
> +  The original convention was
> +
> +  - target `program.cs` builds the .c files of all the modules of `program`
> +  - target `program.os` builds the .o files of all the modules of `program`

The only one of the 's' targets that is publicly documented is .all_ints, which
raises the question of whether we need to announce these changes at all?

The diff looks fine otherwise.

Julien.


More information about the reviews mailing list