[m-rev.] for review: replace item_blocks with parse trees during pre-HLDS passes
jfischer at opturion.com
Fri Mar 13 12:19:22 AEDT 2020
On Fri, 13 Mar 2020, Zoltan Somogyi wrote:
> On Fri, 13 Mar 2020 01:38:07 +1100 (AEDT), Julien Fischer <jfischer at opturion.com> wrote:
>> On Tue, 10 Mar 2020, Zoltan Somogyi wrote:
>>> For review by Julien, since he kindly volunteered.
>>> Thank you in advance. Note that some of the XXXs
>>> in the log message are questions for you. I would
>>> also appreciate it if, besides reviewing the diff, you
>>> also tried it out on the programs you are working on.
>> I have done so and have not encountered any problems. One thing I did
>> encounter due to the changes in warnings for unused modules is:
>> search.m:043: warning: module `fd' is imported in the interface, but it is
>> search.m:043: not used in the interface.
>> search.m:091: Warning: this `:- import_module' declaration for module `fd' in
>> search.m:091: the implementation section is redundant, given the
>> search.m:091: `:- import_module' declaration for the same module in the
>> search.m:091: interface section.
>> search.m:043: The previous declaration is here.
>> We could argubly omit the second warning in cases like the above.
> Agreed, but the problem is that those two warnings are generated by two different
> modules, and the second one printed is actually generated first, so we can't use
> a simple "have we warned about the interface import being unused" test to
> avoid generating the warning about the implementation import being shadowed.
> The only way to handle this would be to delay the generation of the warnings about
> shadowing until *after* module qualification discovers that an interface import
> is not needed, which is not a simple change. I will look into whether it is possible
> to do that at reasonable cost after I commit this diff, but my guess is "no",
> given that any improvement in usability would be minor.
Agreed, I wouldn't do anything about it unless it's trivial to fix.
(The updated version of the compiler found quite a lot of redundant
imports in the code I tested it on; the above only occurred once or
>>> the case where A's interface contains nested submodule A.B's interface, and
>>> A's implementation contains nested submodule A.B's implementation.
>>> Other than that, has been split_parse_tree_src.m any duplicate inclusions
>>> of submodules, whether via nesting or via include_module declarations.
>> What is that last sentence trying to say?
> Yep, that paragraph was sent unfinished. Have a look to see whether the updated
> version in the attached updated log file is readable.
>>> Mention that we now generate warnings for unused import_module and
>>> use_module declarations in the interface even if the module has
>>> XXX Should we report this as a breaking change?
> OK, please have a look at the attached DIFF.NEWS.
> diff --git a/NEWS b/NEWS
> index 46a88fdfa..448094d92 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -11,6 +11,18 @@ Changes that may break compatibility
> simply by importing `one_or_more` as well as (or in rare cases, instead of)
> the `list` module.
> +* If module A contains a `:- import_module` or a `:- use_module` declaration
> + for module B but does not refer to anything defined in module B, we used
> + to generate a warning for the declaration only if module A had no submodules,
> + because it is possible that A's submodules refer to entities defined in B.
> + We now generate a warning for unused `:- import_module` and `:- use_module`
> + declaration in this case as well, which will stop the program from compiling
> + if `--halt-at-warn` is set. The fix is simple: replace the declaration
> + being warned about in the parent module with declarations in the child
> + modules that need it. In the fairly frequent case that not all child modules
> + need module B, this avoids the need to recompile those child modules
> + when the interface of module B changes.
> Changes to the Mercury standard library
> @@ -174,13 +186,33 @@ Changes to the Mercury standard library
> Changes to the Mercury compiler
> +### Changes to the treatment of unused imports
> +* The compiler now generates warnings for import_module and use_module
Mark up import_module etc in the same way as in the previous entry, e.g.
> + declarations in the interface of a module even if that module has
> + submodules. Previously, it generated such warnings only if the module
> + had no submodules.
> ### Changes to code model options
> * `--trail-segments`
> - Grades that support trailing now always use trail segments. This means
> - that the `--trail-segments` option now has no effect, and it is therefore
> - deprecated.
> + Grades that support trailing now always use trail segments. This means
> + that the `--trail-segments` option now has no effect, and it is therefore
> + deprecated.
> +### New warning options
> +* `--print-errors-warnings-when-generating-interface`
> + Until now, the compiler did not try to detect problems with a module
You need a blank line between the item heading and body there.
> + when generating its interface files. Now it does. To preserve compability,
> + by default it still ignores any problems it finds then, but if this option
> + is enabled, and if it does in fact find any problems, the compiler will
> + report them, and if they are severe enough, it will not generate the
> + interface file at all. In the usual use case where the compiler is asked
> + to generate the interface file as part of rebuilding the whole executable,
> + this behavior has the advantage that the rebuild, which would fail anyway,
> + fails sooner.
More information about the reviews