rsmith added a comment.
I would prefer that we have dedicated tests for them rather than scattering the
tests throughout the existing test suite (for example, consider adding
`-Wno-unused` to the tests producing "result unused" warnings and adding a
dedicated test).
================
Comment at: clang/include/clang/Sema/Sema.h:3568-3569
+ /// Corrects typos inside \p SubExprs.
+ ExprResult ActOnSemanticError(SourceLocation Begin, SourceLocation End,
+ ArrayRef<Expr *> SubExprs);
+
----------------
Generally the `ActOn*` interface is invoked by the parser in response to
syntactic constructs, so `ActOnSemanticError` is a surprising function name.
Maybe `CreateRecoveryExpr` would be better. Is that too narrow for the intended
semantics of this function?
================
Comment at: clang/lib/AST/TextNodeDumper.cpp:736-739
+void TextNodeDumper::VisitRecoveryExpr(const RecoveryExpr *Node) {
+ OS << " "
+ << "<recovery-expr>";
+}
----------------
You shouldn't need to print anything here. The class name is automatically
printed for you.
================
Comment at: clang/lib/Sema/SemaExceptionSpec.cpp:1287
// FIXME: Can any of the above throw? If so, when?
return CT_Cannot;
----------------
Should we return `CT_Dependent` for `RecoveryExprClass`, since we model it as
being dependent?
================
Comment at: clang/lib/Sema/SemaExpr.cpp:17978
+ }
+ return RecoveryExpr::Create(Context, Begin, End, NoTypos);
+}
----------------
We shouldn't create these in code synthesis contexts (eg, during SFINAE checks
in template instantiations).
================
Comment at: clang/lib/Sema/TreeTransform.h:9470
+ExprResult TreeTransform<Derived>::TransformRecoveryExpr(RecoveryExpr *E) {
+ return E;
+}
----------------
We should either transform the subexpressions or just return `ExprError()`
here. With this approach, we can violate AST invariants (eg, by having the same
`Expr` reachable through two different code paths in the same function body, or
by retaining `DeclRefExpr`s referring to declarations from the wrong context,
etc).
================
Comment at: clang/test/SemaCXX/builtin-operator-new-delete.cpp:94
void *p = __builtin_operator_new(42, std::align_val_t(2)); // expected-error
{{call to '__builtin_operator_new' selects non-usual allocation function}}
- __builtin_operator_delete(p, std::align_val_t(2)); // expected-error
{{call to '__builtin_operator_delete' selects non-usual deallocation function}}
+ __builtin_operator_delete((void*)0, std::align_val_t(2)); //
expected-error {{call to '__builtin_operator_delete' selects non-usual
deallocation function}}
#endif
----------------
What happens to the original testcase here? I wouldn't expect the invalid call
expression in the initializer of `p` to affect whether we diagnose uses of `p`.
(Nit: remove spaces to realign the comments here.)
================
Comment at: clang/test/SemaOpenCLCXX/address-space-references.cl:15
+ bar(1) // expected-error{{binding reference of type 'const __global unsigned
int' to value of type 'int' changes address space}} \
+ // expected-error{{expected ';' after expression}}
}
----------------
Consider just adding the missing semicolon, so that this test is only testing
the thing it aims to test.
================
Comment at: clang/test/SemaTemplate/instantiate-init.cpp:102
(void)sizeof(array_lengthof(Description<float>::data));
-
+ // expected-warning@+1{{expression result unused}}
sizeof(array_lengthof( // expected-error{{no matching function for call to
'array_lengthof'}}
----------------
Add a cast-to-`void`, like on the previous line.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69330/new/
https://reviews.llvm.org/D69330
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
