Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes #1051 (rename lim_sup -> limn_sup) #1068

Merged
merged 4 commits into from
Oct 30, 2023

Conversation

affeldt-aist
Copy link
Member

Motivation for this change

fixes #1051

as (briefly) discussed during the last meeting https://github.com/math-comp/analysis/wiki/2023-10-16-Meeting

Things done/to do
  • added corresponding entries in CHANGELOG_UNRELEASED.md
  • added corresponding documentation in the headers
Compatibility with MathComp 2.0
  • I added the label TODO: HB port to make sure someone ports this PR to
    the hierarchy-builder branch or I already opened an issue or PR (please cross reference).
Automatic note to reviewers

Read this Checklist and put a milestone if possible.

@affeldt-aist affeldt-aist added the TODO: MC2 port This PR must be ported to mathcomp 2 now that the. Remove this label when the port is done. label Oct 23, 2023
@affeldt-aist affeldt-aist added this to the 0.6.6 milestone Oct 23, 2023
@affeldt-aist affeldt-aist changed the title fixes #1051 (rename lim_sup -> lim_nsup) fixes #1051 (rename lim_sup -> limn_sup) Oct 23, 2023
@affeldt-aist affeldt-aist marked this pull request as ready for review October 23, 2023 14:02
@affeldt-aist
Copy link
Member Author

affeldt-aist commented Oct 24, 2023

TODO: add only parsing to deprecated notations! fixed (at least in the files concerned by this PR)

@affeldt-aist
Copy link
Member Author

@CohenCyril the renaming is as we discussed during the last meeting, plus one name change that I made to be more uniform between the real and the ereal versions
I put "only parsing" format info for all deprecated (they shoud have been here from the start)

@@ -1459,17 +1459,17 @@ End standard_measurable_fun.
#[global] Hint Extern 0 (measurable_fun _ (fun x => x ^+ _)) =>
solve [exact: measurable_exprn] : core.
#[deprecated(since="mathcomp-analysis 0.6.3", note="use `measurable_exprn` instead")]
Notation measurable_fun_sqr := measurable_exprn.
Notation measurable_fun_sqr := measurable_exprn (only parsing).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know Coq accepted this... (only parsing syndefs...)

@CohenCyril
Copy link
Member

Sure! I agree with the renaming. Can you tell me why the "only parsing" for lemmas (where are they printed anyway?)

@affeldt-aist
Copy link
Member Author

affeldt-aist commented Oct 25, 2023

I think that I only added them ("only parsing" format info) to Notation's. Indeed, I don't expect deprecation to work for lemma (I saw a few such faulty use of the deprecated pragmas in Rstruct.v
that I didn't try to fix).

@CohenCyril
Copy link
Member

I meant: why bother putting only parsing on notations for lemma. What else than Print is going to display them anyway

@affeldt-aist
Copy link
Member Author

I'll remove them asap.

@affeldt-aist
Copy link
Member Author

I'll remove them asap.

Well, in fact, besides Print, they are useful to have correct error display.
If you apply the lemma for which a deprecated notation is not only parsing,
the error will report about the deprecated notation instead of the right lemma,
which is a bit disturbing (it happens a lot with measurable_* lemmas).

@CohenCyril
Copy link
Member

CohenCyril commented Oct 25, 2023

I'll remove them asap.

Well, in fact, besides Print, they are useful to have correct error display. If you apply the lemma for which a deprecated notation is not only parsing, the error will report about the deprecated notation instead of the right lemma, which is a bit disturbing (it happens a lot with measurable_* lemmas).

I do not understand. It's good that the error is about the deprecated lemma, and that the warning notice gives the new lemma. Can you give me a concrete usecase, with a concrete error message and what's wrong with it. I tried to reproduce what you said but for me the result is unchanged whether I put only parsing or not...

e.g. whether I put measurable_fun_sqr as only parsing or not I get the following message, when I replace lebeqgue_measure.v line 1507, with:

   by apply: measurableT_comp (measurable_fun_sqr _) _; exact: measurable_funD.

it emits:

Warning: Notation measurable_fun_sqr is deprecated since mathcomp-analysis 0.6.3.
use `measurable_exprn` instead [deprecated-syntactic-definition,deprecated]

anyway...

@affeldt-aist
Copy link
Member Author

For example, in the current state, if you apply, say, measurableT_comp in the second half of lebesgue_measure.v, if it fails, it fails with the error

Error: Cannot apply lemma emeasurable_funN

which is a bit puzzling at first sight.
With only parsing, at least the error message is

Error: Cannot apply lemma measurableT_comp

@CohenCyril
Copy link
Member

CohenCyril commented Oct 25, 2023

Oh I undestand now!! Thanks for the clarification!

@CohenCyril
Copy link
Member

This is really nontrivial, could you document it somewhere?

@affeldt-aist
Copy link
Member Author

This is really nontrivial, could you document it somewhere?

What about an entry in CONTRIBUTING.md about how to write deprecated pragmas?

@affeldt-aist
Copy link
Member Author

I added a few sentence in CONTRIBUTING.md

@affeldt-aist affeldt-aist merged commit 270c4c4 into math-comp:master Oct 30, 2023
30 checks passed
affeldt-aist added a commit to affeldt-aist/analysis that referenced this pull request Nov 10, 2023
proux01 pushed a commit that referenced this pull request Nov 10, 2023
* fixes #1051

* add only parsing
@proux01 proux01 removed the TODO: MC2 port This PR must be ported to mathcomp 2 now that the. Remove this label when the port is done. label Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lim_sup -> limn_sup
3 participants