Community
Participate
Working Groups
I20080918-0100 IMemberValuePair#getValue() should also work for negative numerals. Currently, it only works for positive and hex numbers. @Ints({1, 0xDadaCafe, -1}) @interface Ints { int[] value(); } @Doubles({1.2e42, 0xFBC.12Fp-1, -1.2,-0xFBC.12Fp-1}) @interface Doubles { double[] value(); } IField#getConstant() works fine under the same circumstances. Not urgent for me, since I decided in the meantime that I will use the IAnnotationBinding anyway (for rendering in hovers).
-1, -1.2, etc... are not literals. They are UnaryExpressions, i.e. they are covered by "the value is an expression that would need to be further analyzed to determine its kind" part of the spec. However we could indeed support those with a little more work.
Please investigate.
Created attachment 150063 [details] proposed fix v0.5 + regression test Created 4 new kinds in IMemberValuePair - K_NEGATIVE_INT, K_NEGATIVE_FLOAT, K_NEGATIVE_LONG, K_NEGATIVE_DOUBLE. The new method Util.getNegativeAnnotationMemberValue() handles the negative numerals, creating the member value and setting the value kind out of the above 4 new kinds.
(In reply to comment #3) > Created an attachment (id=150063) [details] > proposed fix v0.5 + regression test > > Created 4 new kinds in IMemberValuePair - K_NEGATIVE_INT, K_NEGATIVE_FLOAT, > K_NEGATIVE_LONG, K_NEGATIVE_DOUBLE. The new method > Util.getNegativeAnnotationMemberValue() handles the negative numerals, creating > the member value and setting the value kind out of the above 4 new kinds. First, some remarks while reading the code contents: 1. you should use the same code formatting than the one usually used in the code around your addition. In JDT/Core team we use the Eclipse built-in profile. It means that: a. the line comment should have a space between the leading '//' and the first word of the comment, b. there's a space afterthe 'if' and its condition, 2. more specifically, in the algorithm added in LocalVariable.getAnnotationMemberValue(...) and in CompilationUnitStructureRequestor.getMemberValue(...) methods, you should put the UnaryExpression in a local variable to avoid to make the cast twice on 'expression' BTW, I wonder whether it wouldn't have been better to simply add a K_NEGATIVE flag (e.g. 0x100) to the IMemberValuePair interface and just set this flag on the value kind for negative values. E.g.: /** * Flag indicating that the value is negative. It only can be set for * {@link #K_INT}, {@link #K_FLOAT}, {@link #K_DOUBLE} and {@link #K_LONG} * kinds of value. */ int K_NEGATIVE = 0x100; As this interface is not supposed to be implemented or extended by clients, then this couldn't be considered as a breakage... (hmm, I noticed that this class is currently missing the @noextend and @noimplement flags! May be this could be fixed while fixing this bug...) Markus, what would be the preferable API for you, the four added specific constants or the single flag for negative values?
> First, some remarks while reading the code contents: Thanx. I've noted down the remarks and made the necessary corrections. > BTW, I wonder whether it wouldn't have been better to simply add a K_NEGATIVE > flag (e.g. 0x100) to the IMemberValuePair interface and just set this flag on > the value kind for negative values. There wasnt any harm in being more specific. I thought adding 4 different flags would add more clarity. I'll wait for Markus' opinion and then submit the changes.
(In reply to comment #1) > -1, -1.2, etc... are not literals. [..] That is an interesting implementation detail, but it should not matter for clients of the Java model. The result set of getValueKind() as specified in the Javadoc is closed, so adding more constants or a mask would be a breaking change. But luckily, no breakage is necessary here at all. You should just support these UnaryExpressions internally and return K_INT, K_FLOAT, etc. also for negative numbers. getValue() can also perfectly box the negative values. BTW: I just saw a typo in the Javadoc of K_UNKNOWN: It should be @MyAnnot({1, 2.3, "abc"}) , not @MyAnnot(1, 2.3, "abc")
Created attachment 151184 [details] proposed fix v1.0 + regression test Fixed formatting problems. Fixed Javadoc for K_UNKNOWN. Removed all 4 new kinds. Using the old K_INT, K_FLOAT, etc. now to box negative values as well.
The last patch fails with CCEs for e.g. this code: @Bytes({0, -1, - Byte.MIN_VALUE}) @interface Bytes { byte[] value(); } With the necessary 2 instanceofs added, it behaves as expected. java.lang.ClassCastException: org.eclipse.jdt.internal.compiler.ast.QualifiedNameReference cannot be cast to org.eclipse.jdt.internal.compiler.ast.Literal at org.eclipse.jdt.internal.core.CompilationUnitStructureRequestor.getMemberValue(CompilationUnitStructureRequestor.java:727) at org.eclipse.jdt.internal.core.CompilationUnitStructureRequestor.getMemberValue(CompilationUnitStructureRequestor.java:714) at org.eclipse.jdt.internal.core.CompilationUnitStructureRequestor.getMemberValuePair(CompilationUnitStructureRequestor.java:658) ...
Ayushman, please provide a new patch based on Markus's comment (see comment 8).
Created attachment 151680 [details] proposed fix with correction + regression tests The earlier patch with changes to support qualified name references. Modified the tests a bit to reflect the chage.
(In reply to comment #10) > Created an attachment (id=151680) [details] > proposed fix with correction + regression tests > > The earlier patch with changes to support qualified name references. Modified > the tests a bit to reflect the chage. Please remove the special code for negated qualified name references again. With the '-', it's not a qualified name reference any more, so the kind should stay K_UNKNOWN.
Created attachment 151780 [details] proposed fix v2.0 + regression tests Returning K_UNKNOWN as the kind for sign prefixed qualified references. Removed the concerned code as mentioned in the above comment.
Patch looks good. Note that the ClassFileTests passed without the fix. It is only the CompilationUnitTests that are affected by this change. I'll keep the new tests inside ClassFileTests anyway as they explicitly test negative primitive values. I'll release it today.
I adjusted two copyrights when releasing the patch.
Verified for 3.6M4 using Build id: I20091207-1800
