Detect overlapping field names #147
Conversation
| stderr: | ||
| ...test.som', line 10, column 23: | ||
| test = test_super ( | a | | ||
| Field 'a' is already defined in a superclass. |
There was a problem hiding this comment.
I'm not sure if this should be "Field" or (as below) "Field name". Any thoughts?
There was a problem hiding this comment.
I as non-native speak would say: 'Field a is already defined' or 'A field with the name a is already defined'.
"Field name 'a' has already been defined in this class." feels wrong.
Perhaps 'A field named 'a' ...' as another option.
There was a problem hiding this comment.
Yes, I think on reflection that "Field" is better than "Field name" (due to the fields method being a precedent).
There was a problem hiding this comment.
Or even better "A field by the name 'a' is already defined...".
There was a problem hiding this comment.
The most useful improvement would IMHO be to name the superclass and the source location where the conflicting field is defined. That would be very developer-friendly :)
There was a problem hiding this comment.
I don't think error messages need to use full grammar (so to speak): it can often end up being pointless extra verbiage to plough through especially when you tend to see the same messages over and over again. I think I'd prefer to keep it as in f57c74b.
| stderr: | ||
| ...test.som', line 10, column 23: | ||
| test = test_super ( | a | | ||
| Field 'a' is already defined in a superclass. |
There was a problem hiding this comment.
I as non-native speak would say: 'Field a is already defined' or 'A field with the name a is already defined'.
"Field name 'a' has already been defined in this class." feels wrong.
Perhaps 'A field named 'a' ...' as another option.
| stderr: | ||
| ...instance_fields_overlap2.som', line 11, column 9: | ||
| | a a | | ||
| Field name 'a' has already been defined in this class. |
There was a problem hiding this comment.
One thing I am surprised by is that you don't format the error message in a "standard way" with path:line:column, which is supported by a large number of tools for navigation to the error.
There was a problem hiding this comment.
I didn't know that's a standard to be honest! Changing the formatting is pretty trivial if anyone wants to raise a PR for it (it's not something I feel very strongly about either way), with the only proviso that I think the console output should still look reasonable for humans.
There was a problem hiding this comment.
It's not really a standard, just a defacto thing a lot of tools support.
Not to insult anyone, but if I remember correctly, you're using vim.
In vim, for instance with :make you get :cnext etc to navigate to errors.
That's simply taking the test.c:4:3: error: use of undeclared identifier 'bax' message to work.
In many IDEs, these lines are recognized and clickable to jump to errors.
So, all I am saying it may even make your own life easier :)
|
A handful of comments. |
|
I tend to agree with @smarr that this PR isn't about duplicate superclasses (SOM doesn't have the diamond problem). |
|
Agreed. I think I'm happy with this. Shall I merge? |
|
Ah, please squash first. |
|
I need to squash if that's OK? |
|
Squashed. |
ltratt
force-pushed
the
overlapping_fields
branch
from
735b752 to
4d6ba76
Compare
|
bors r+ |
|
I realise that some of my comments from earlier didn't make a great deal of sense. Maybe another coffee before my next PR review! My bad. |
|
It's not your fault -- I don't expect you to know SOM inside out (I'm only starting to slowly get to grips with it myself!). |
|
Build succeeded: |
There are two cases to be dealt with:
The first of those was pointed out by @Hirevo in SOM-st/SOM#42; the second is a relatively simple variation on the same theme.