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

Fix PR7040: report duplicate methods in a class, also for virtual methods #290

Closed
wants to merge 1 commit into from

Conversation

alainfrisch
Copy link
Contributor

See http://caml.inria.fr/mantis/view.php?id=7040

There is already an error that reports duplicated methods in a class:

# class c = object method x = 1 method x = 2 end;;
Characters 30-42:
  class c = object method x = 1 method x = 2 end;;
                                ^^^^^^^^^^^^
Error: The method `x' has multiple definitions in this object

This PR triggers the same error in case of duplication between two virtual methods or one virtual and one non-virtual method.

@gasche
Copy link
Member

gasche commented Nov 13, 2015

When you add a Changes entry, it should marked as "may break programs", because it is more than conceivable that people would have used virtual methods as prototypes to pre-declare their methods for readability purposes.

@alainfrisch
Copy link
Contributor Author

because it is more than conceivable that people would have used virtual methods as prototypes to pre-declare their methods for readability purposes.

Good point. Would you be in favor of not adding this stricter check, then, if this use case is considered valid?

@gasche
Copy link
Member

gasche commented Nov 13, 2015

Indeed, I would think that this stricter check is not worth it: it breaks reasonable programs while it doesn't prevent any kind of dangerous situation, or help users in another way. The virtual-virtual case, on the other hand, seems completely useless and could deserve at least a warning, but you may also just not bother.

@garrigue
Copy link
Contributor

Even two virtual methods could have some meaning: this forces unification between the two types.
So I agree that it is better no to try to guess what the user is doing.

@alainfrisch
Copy link
Contributor Author

I'm convinced by these arguments. Let's do nothing here.

mshinwell pushed a commit to mshinwell/ocaml that referenced this pull request Feb 11, 2021
Improvements to the typing operations
chambart pushed a commit to chambart/ocaml-1 that referenced this pull request Jan 4, 2022
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.

None yet

3 participants