Code cleanup in notations: use records instead of tuples for entries and subentries #17823
Conversation
This file contains 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
herbelin
added
kind: cleanup
coqbot-app
bot
added
the
needs: rebase
…tuples. See comment at coq#17117 (comment).
herbelin
force-pushed
the
master+cleanup-use-record-for-notation_entry_level
branch
from
0b9986c
to
99ed573
Compare
coqbot-app
bot
removed
needs: rebase
ejgallego
reviewed
Jul 6, 2023
| type notation_entry_level = { | ||
| notation_entry : notation_entry; | ||
| notation_level : entry_level; | ||
| } |
There was a problem hiding this comment.
@herbelin if you have the energy, the form:
module Entry_level = struct
type t = { entry : notation_entry;
; level : entry_level
}
end
tends to work better.
There was a problem hiding this comment.
I don't buy the "; ..." formatting which moreover is not the most "standard" way to format records in the code of Coq.
Otherwise, what are the pros and cons of using a module? And how do you measure the "better"?
There was a problem hiding this comment.
Using a module only makes sense if you have functions directly related to it together. If it is just a one off type its probably not worth it.
coqbot-app
bot
added
the
needs: full CI
|
@coqbot run full ci |
coqbot-app
bot
removed
the
needs: full CI
2 tasks
|
The job library:ci-fiat_crypto_legacy has failed in allow failure mode |
proux01
approved these changes
Jul 8, 2023
|
@coqbot merge now |
|
@proux01: Please take care of the following overlays:
|
gares
added a commit
to LPCIC/coq-elpi
that referenced
this pull request
Jul 9, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This was suggested by @proux01 in this comment of #17117 and this makes explicit when entries (equivalent of
in entry at level nn) and subentries (equivalent ofid entry at level nn) matter in the code.No semantic change.
Overlay: