eclipse-aspectj / aspectj

Other
297 stars 86 forks source link

Rename & fix or delete 'InterTypeClassHeaderName1' grammar rule #208

Open kriegaex opened 1 year ago

kriegaex commented 1 year ago

When fixing another grammar problem, I noticed Jikespg output as follows:

The following Non-Terminal is useless: InterTypeClassHeaderName1

Therefore, I added a TODO comment in https://github.com/eclipse/aspectj.eclipse.jdt.core/commit/f2a47a37be1abb5ed7cdc78ff6f896c23c708b92, see also directly in the file:

-- TODO: Rename 'InterType*' to 'Intertype*'? Without renaming, Jikespg says:
--   The following Non-Terminal is useless: InterTypeClassHeaderName1
-- But after renaming, it says instead:
--   Reduce/reduce conflict on "LESS" between rule 410 and 303
--   Reduce/reduce conflict on "LESS" between rule 405 and 304
InterTypeClassHeaderName1 ::= Modifiersopt 'class' OnType TypeParametersAsReference '.' JavaIdentifier
/.$putCase consumeIntertypeClassHeaderName(true); $break ./
/:$readableName IntertypeClassHeader:/

@aclement, can you please review this portion of the grammar and tell me what is supposed to happen there? Is that rule (production) superfluous or supposed to be fixed?

aclement commented 1 year ago

I'm afraid I don't speak jikespg. That area of the project is extremely fragile as since the original work X number of years ago (not by me), it has not been touched other than in emergencies and then we rely on (a) can we get a valid file out (whether there are warnings or not) and (b) does it pass all the tests. Another reason to switch to plain java with annotations and away from JDT fork.

kriegaex commented 1 year ago

@aclement: Well, if you simply look at the grammar rules, you notice that due to upper vs. lower case spelling (typo?) in the commented rule, it is not being referenced elsewhere. The rule in question is different from the preceding one in that it caters to generics, i.e. something like ITDs like public class XY<T>.foo, while the preceding rule IntertypeClassHeaderName1 (lower-case "t" in "Intertype") seems to cover the non-generic case public class XY.foo. I.e., there might be something not working which is supposed to syntax-wise. I thought that maybe you could shed light on this issue.

aclement commented 1 year ago

It sounds like you are correct. I don't recall. As I say, the tests are the usual arbiter on whether things are working. We might well not have all the tests we need around generics and ITDs.