[PATCH] D82954: Fix crash on overloaded postfix unary operators due to invalid SourceLocation

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added inline comments.


================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:118
 
+syntax::NodeKind getOperatorNodeKind(const CXXOperatorCallExpr &E) {
+  switch (E.getOperator()) {
----------------
eduucaldas wrote:
> # Where to put this logic? 
> The pro of having this function inside `BuildTree.cpp` is that it is closer to it's *only* usage. And also for now `BuildTree.cpp` agglomerates everything that takes a semantic AST as an input, so it would be coherent.
> 
> Another option is to put it in `Nodes`, as it might serve as documentation for `*OperatorExpression`s. 
> For example, it would document that the semantic node `CXXOperatorCallExpr` can also generate the syntax node `BinaryOperatorExpression`, which was previously only generated by a semantic `BinaryOperator`
> 
> Another option is to add put this function as a lambda inside `WalkUpFromCXXOperatorCallExpr` as probably it will only be used there. 
Placing this helper here makes sense to me. However, please mark it static.


================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:178
+    return syntax::NodeKind::BinaryOperatorExpression;
+  // Not supported by SyntaxTree
+  case OO_New:
----------------
"Not supported by syntax tree *yet*"


================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:741
   bool WalkUpFromIntegerLiteral(IntegerLiteral *S) {
+    if (S->getLocation().isInvalid())
+      return true;
----------------
WDYT about overriding `TraverseCXXOperatorCallExpr`, so that we don't even visit the synthetic argument of the postfix unary `++`? I would prefer to not introduce blanket "if invalid then ignore" checks in the code.


================
Comment at: clang/lib/Tooling/Syntax/BuildTree.cpp:852
+    default:
+      llvm_unreachable("getKind does not implement that");
     }
----------------
"getOperatorNodeKind() does not return this value"


================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:2333
+struct X {
+  X operator++(int);
+};
----------------
Could you also add tests for a prefix unary operator?


================
Comment at: clang/unittests/Tooling/Syntax/TreeTest.cpp:2376
+    | | |   `-x
+    | | `-IdExpression
+    | |   `-UnqualifiedId
----------------
I'm not sure about this part where `++` is wrapped in IdExpression -- shouldn't the syntax tree look identical to a builtin postfix `++` (see another test in this file)?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82954/new/

https://reviews.llvm.org/D82954