mibintc marked 9 inline comments as done.
mibintc added a comment.
A couple inline replies to go along with the updated patch
================
Comment at: clang/include/clang/Driver/Options.td:1757
+ LangOpts<"ProtectParens">, DefaultFalse,
+ PosFlag<SetTrue, [CC1Option],
+ "Determines whether the optimizer honors parentheses when "
----------------
aaron.ballman wrote:
> Should this option also be exposed to clang-cl (should it be a `CoreOption`)?
I don't know the answer to this, do you have a recommendation?
================
Comment at: clang/lib/Sema/SemaChecking.cpp:6427-6430
+ if (const VectorType *VT = dyn_cast<VectorType>(ArgTy.getCanonicalType()))
+ return VT->getElementType().getTypePtr()->isFloatingType();
+ if (const ComplexType *CT =
dyn_cast<ComplexType>(ArgTy.getCanonicalType()))
+ return CT->getElementType().getTypePtr()->isFloatingType();
----------------
aaron.ballman wrote:
>
I found there was a boolean function, so i got rid of the lambda.
================
Comment at: clang/lib/Sema/SemaCoroutine.cpp:294
-static Expr *buildBuiltinCall(Sema &S, SourceLocation Loc, Builtin::ID Id,
- MultiExprArg CallArgs) {
----------------
I moved this function to public because another file is also using the same
logic
================
Comment at: clang/lib/Sema/SemaExpr.cpp:4026
+ !E->isLValue() &&
+ (ExprTy->isFloatingType() || (ExprTy->isComplexType()))) {
+ return BuildBuiltinCallExpr(R, Builtin::BI__arithmetic_fence, E);
----------------
aaron.ballman wrote:
> Do you have to worry about vector types here as well?
Oh yes, i didn't get this one.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:4027
+ (ExprTy->isFloatingType() || (ExprTy->isComplexType()))) {
+ return BuildBuiltinCallExpr(R, Builtin::BI__arithmetic_fence, E);
+ }
----------------
aaron.ballman wrote:
> One worry I have about this is that the AST will be a bit mysterious for
> people writing AST matchers. The source code will have parens in it, but the
> AST will have a `ParenExpr` node or not only depending on the language
> options. This may also cause problems for other parts of the code that are
> checking for properly-parenthesized expressions (like && and || within the
> same expression), so we should probably have a test case like:
> ```
> bool func(float f1, float f2, float f3) {
> return (f1 == f2 && f1 == f3) || f2 == f3; // Should not warn here
> }
> ```
I added an AST test case
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D100118/new/
https://reviews.llvm.org/D100118
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
