Disable warning 66 (unused-open-bang) locally rather than globally#11498
Merged
dra27 merged 3 commits intoocaml:trunkfrom Aug 24, 2022
Merged
Disable warning 66 (unused-open-bang) locally rather than globally#11498dra27 merged 3 commits intoocaml:trunkfrom
dra27 merged 3 commits intoocaml:trunkfrom
Conversation
But make sure not to turn it into an error at this stage, so that all the warnings get printed.
This commit disables warning 66 (unused-open-bang) only locally in the four places where it was triggered, rather than disabling it for the whole compiler. See the source code for the explanation about why it is necessary to use code that triggers this warning.
Now that the warning has been disabled exactly where it was currently necessary to disable it, we can turn it into an error in the build system.
dra27
approved these changes
Aug 24, 2022
Member
dra27
left a comment
There was a problem hiding this comment.
Definitely nicer this way, yes!
Contributor
Author
|
David Allsopp (2022/08/24 07:29 -0700):
@dra27 approved this pull request.
Definitely nicer this way, yes!
Cool! Thanks!
|
Contributor
Author
|
David Allsopp (2022/08/24 07:30 -0700):
Merged #11498 into trunk.
And thanks for having merged it!!
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
At the moment, our build system imposes that the dependencies computed
by ocamldep for the native backends must be exactly the same for
all backends, although the dependencies are actually different.
To circumvent this limitation, four backends (amd64, arm64, i386
and riscv) need to introduce a dependency that does not really exist:
these backends don't have instruction scheduling enabled, but still, their
scheduling.mlmodule mustopen! Schedgento cope with the requirements of our build system.On top of that, when it is enabled, the warning 66 (unused-open-bang)
rightly reports that the above mentionned directives are not necessary.
In the current codebase, this warning is disabled at the level
of the build system, so for the whole compiler's codebase.
The purpose of this PR is to disable the warning locally rather than
globally, only at those places where it is known that, for the moment,
it is necessary to do so.
The PR is not an ultimate fix. The ultimate fix would consist in
having a build system which tolerates that each backend has its own
dependencies, which does not necessarily entail that the dependencies
can not be stored statically. But that's a bit too ambitious, which
would likely require more invasive changes and so this PR proposes
an intermediate solution, a middle way so to speak, which can be summed
up as follows: we are still hiding the dust under a carpet but this
PR makes the carpet smaller than it was before, with the hope that, one day,
it will become possible to remove both carpet and dust.
the other purpose of this PR is to reduce the difference between the
warnings WHICH NEED TO BE disabled to compile the compiler and
those which need to be disabled to compile ocamllex, to make
the merge of
lex/Makefileinto the rootMakefileproposed in#11420 easier.
Of course, in theory it could make sense to different different lists of
warnings to compile the compiler and ocamllex. But, in the particular
case addressed by this PR, it does not sound insane to turn a global
warning into a local one. The Pr also clarifies the situation in
the codebase itself, thanks to the comments and to an attribute,
which is of course also a form of documentation.