Skip to content

add support for binary operator trees in annotations - #182

Merged
Strum355 merged 1 commit into
mainfrom
nsc/annotation-tree-fix178
Apr 23, 2021
Merged

add support for binary operator trees in annotations#182
Strum355 merged 1 commit into
mainfrom
nsc/annotation-tree-fix178

Conversation

@Strum355

Copy link
Copy Markdown
Contributor

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

@olafurpg olafurpg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

binOp.setOp(Semanticdb.BinaryOperator.MODULO);
break;
case LESS_THAN:
binOp.setOp(Semanticdb.BinaryOperator.LESSER);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no motivation for the names, am completely not beholden to the choices 🙂

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the absence of opinions I think it's best to use the same names as the compiler

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread semanticdb-java/src/main/protobuf/semanticdb.proto Outdated
@Strum355
Strum355 force-pushed the nsc/annotation-tree-fix178 branch 2 times, most recently from 69a451d to 088de87 Compare April 23, 2021 15:34
@Strum355
Strum355 force-pushed the nsc/annotation-tree-fix178 branch from 088de87 to 30114ed Compare April 23, 2021 15:36

@olafurpg olafurpg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🔥

@Strum355

Copy link
Copy Markdown
Contributor Author

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

@Strum355
Strum355 merged commit 339a445 into main Apr 23, 2021
@Strum355
Strum355 deleted the nsc/annotation-tree-fix178 branch April 23, 2021 16:21
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.

Exception when building annotation tree in netflix/genie

2 participants