add support for binary operator trees in annotations #182
Conversation
olafurpg
left a comment
olafurpg
left a comment
There was a problem hiding this comment.
Nice! I didn't know Java supported expressions inside annotations, but I'm not surprised. A few minor questions about the names, otherwise this looks good
semanticdb-javac/src/main/java/com/sourcegraph/semanticdb_javac/SemanticdbVisitor.java
Outdated
Show resolved
Hide resolved
| binOp.setOp(Semanticdb.BinaryOperator.MODULO); | ||
| break; | ||
| case LESS_THAN: | ||
| binOp.setOp(Semanticdb.BinaryOperator.LESSER); |
There was a problem hiding this comment.
Is there a motivation for using LESSER instead of LESS_THAN? What do you think about using the same names as the ones from the compiler?
There was a problem hiding this comment.
no motivation for the names, am completely not beholden to the choices 🙂
There was a problem hiding this comment.
In the absence of opinions I think it's best to use the same names as the compiler
There was a problem hiding this comment.
Ive updated a few of the names to closer match the compiler. For the shift operators, Ive taken some "creative freedom" to allow for imo nicer prefix-grouping.
Wouldve liked to group XOR/OR/AND, but theyre not just bitwise but logical too https://docs.oracle.com/javase/specs/jls/se16/html/jls-15.html#jls-15.22
semanticdb-javac/src/main/java/com/sourcegraph/semanticdb_javac/SemanticdbVisitor.java
Outdated
Show resolved
Hide resolved
Strum355
force-pushed
the
nsc/annotation-tree-fix178
branch
2 times, most recently
from
69a451d to
088de87
Compare
Strum355
force-pushed
the
nsc/annotation-tree-fix178
branch
from
088de87 to
30114ed
Compare
|
Just as a heads up, ternary operators are still not included. If we come across actual usage of it (none found with a sourcegraph search), then we can look into that |
Previously, lsif-java would throw an exception for annotations in the form such as
@Annotation(IDENT + literal)or any other binary operation AST.This PR adds more to be upstreamed in scalameta/scalameta#2281
Closes #178