# Changeset View

# Standalone View

# lib/Sema/SemaExpr.cpp

- This file is larger than 256 KB, so syntax highlighting is disabled by default.

Show First 20 Lines • Show All 10884 Lines • ▼ Show 20 Line(s) | 10883 | if (BinaryOperator::isEqualityOp(Opc) && | |||
---|---|---|---|---|---|

10885 | assert(RHS.get()->getType()->hasFloatingRepresentation()); | 10885 | assert(RHS.get()->getType()->hasFloatingRepresentation()); | ||

10886 | CheckFloatComparison(Loc, LHS.get(), RHS.get()); | 10886 | CheckFloatComparison(Loc, LHS.get(), RHS.get()); | ||

10887 | } | 10887 | } | ||

10888 | 10888 | | |||

10889 | // Return a signed type for the vector. | 10889 | // Return a signed type for the vector. | ||

10890 | return GetSignedVectorType(vType); | 10890 | return GetSignedVectorType(vType); | ||

10891 | } | 10891 | } | ||

10892 | 10892 | | |||

10893 | static void diagnoseXorMisusedAsPow(Sema &S, ExprResult &LHS, ExprResult &RHS, | ||||

10894 | SourceLocation Loc) { | ||||

10895 | auto *LHSInt = dyn_cast<IntegerLiteral>(LHS.get()); | ||||

aaron.ballman: Missing a full stop at the end of the comment. Same comment applies elsewhere. | |||||

10896 | auto *RHSInt = dyn_cast<IntegerLiteral>(RHS.get()); | ||||

10897 | if (!LHSInt || !RHSInt) | ||||

10898 | return; | ||||

10899 | | ||||

10900 | if (LHSInt->getValue().getBitWidth() != RHSInt->getValue().getBitWidth()) | ||||

10901 | return; | ||||

10902 | | ||||

10903 | CharSourceRange OpRange = CharSourceRange::getCharRange( | ||||

10904 | LHSInt->getBeginLoc(), S.getLocForEndOfToken(RHSInt->getLocation())); | ||||

10905 | llvm::StringRef ExprStr = | ||||

10906 | Lexer::getSourceText(OpRange, S.getSourceManager(), S.getLangOpts()); | ||||

10907 | | ||||

10908 | if (S.getLangOpts().CPlusPlus) { | ||||

10909 | // Do not diagnose binary literals | ||||

10910 | if (ExprStr.find("0b") != llvm::StringRef::npos) | ||||

Done Reply
aaron.ballman: `if (const auto *UO = dyn_cast<UnaryOperator>(RHS.get())) { ... }` rather than doing an `isa<>`… | |||||

10911 | return; | ||||

10912 | // Do not diagnose if xor keyword is used | ||||

10913 | if (ExprStr.find("xor") != llvm::StringRef::npos) | ||||

Done ReplyWhy do you special-case Quuxplusone: Why do you special-case `0b` prefix (or suffix — `0x0b` is also special-cased by this snippet)… | |||||

10914 | return; | ||||

10915 | } | ||||

10916 | | ||||

10917 | const llvm::APInt &LeftSideValue = LHSInt->getValue(); | ||||

10918 | const llvm::APInt &RightSideValue = RHSInt->getValue(); | ||||

10919 | const int64_t RightSideIntValue = RightSideValue.getSExtValue(); | ||||

10920 | if (RightSideIntValue < 1) | ||||

10921 | return; | ||||

10922 | | ||||

10923 | if (LeftSideValue == 2 || LeftSideValue == 10) { | ||||

10924 | llvm::APInt XorValue = LeftSideValue ^ RightSideValue; | ||||

Done ReplyDo you have metrics indicating that this line is an improvement over Quuxplusone: Do you have metrics indicating that this line is an improvement over `if (true) {`? | |||||

Done ReplyThis is left over of older code :) I will fix it. xbolva00: This is left over of older code :) I will fix it. | |||||

10925 | std::string LHSStr = Lexer::getSourceText( | ||||

10926 | CharSourceRange::getTokenRange(LHSInt->getSourceRange()), | ||||

10927 | S.getSourceManager(), S.getLangOpts()); | ||||

Done ReplyWhy is this C++ only? C has an aaron.ballman: Why is this C++ only? C has an `xor` macro that comes from `iso646.h`. Should add a test case… | |||||

10928 | std::string RHSStr = Lexer::getSourceText( | ||||

10929 | CharSourceRange::getTokenRange(RHSInt->getSourceRange()), | ||||

Not Done ReplyDoesn't this match any expression that contains jfb: Doesn't this match any expression that contains `xor`? Put another way, I don't see `"xor"`… | |||||

Done ReplyYes, but since xor is keyword in C++, I think this is safe. xbolva00: Yes, but since xor is keyword in C++, I think this is safe. | |||||

Not Done ReplyI believe JF is worried about expressions like Quuxplusone: I believe JF is worried about expressions like `constexpr int flexor_exponent = 3; return 10 ^… | |||||

Done ReplyAh, I will change it to “ xor “. xbolva00: Ah, I will change it to “ xor “. | |||||

Not Done ReplySpaces aren't the only valid whitespace character :) 2 xor 31 jfb: Spaces aren't the only valid whitespace character :)
```
2 xor
31
``` | |||||

Done ReplyI will rework it to chech only XOR op, not the while expr. xbolva00: I will rework it to chech only XOR op, not the while expr. | |||||

Done ReplyWell, 10 ^ flexor .. We cannot reach xor check, flexor is not a integer literal. So I will rework nothing here. xbolva00: Well, 10 ^ flexor ..
We cannot reach xor check, flexor is not a integer literal. So I will… | |||||

Done Reply#define flexor 7 Now aaron.ballman: ```
#define flexor 7
```
Now `flexor` is an integer literal. (A test case for this would be… | |||||

Done ReplyAlso, as I've said above: constexpr long long operator"" _xor(unsigned long long v) { return v; } auto i = 10 ^ 5_xor; jfb: Also, as I've said above:
```
constexpr long long operator"" _xor(unsigned long long v)
{… | |||||

10930 | S.getSourceManager(), S.getLangOpts()); | ||||

10931 | if (LeftSideValue == 2) { | ||||

Done ReplyCan you use Hex and binary are handled up here on line 10927, but octal is handled down on line 10955; why? Can't they be combined into one place in the code? Quuxplusone: Can you use `starts_with` (or the LLVM equivalent) in both of these cases? It'll be faster and… | |||||

Done ReplyWe cannot use starts_with here, case: 2 ^ 0b11. Yes, I can combine it to one place. xbolva00: We cannot use starts_with here, case: 2 ^ 0b11.
Yes, I can combine it to one place.
| |||||

10932 | llvm::APInt PowValue = (LeftSideValue - 1) << RightSideValue; | ||||

10933 | std::string SuggestedExpr = "1<<" + RHSStr; | ||||

10934 | S.Diag(Loc, diag::warn_xor_used_as_pow_base_two) | ||||

10935 | << ExprStr << XorValue.toString(10, true) << SuggestedExpr | ||||

10936 | << PowValue.toString(10, true) | ||||

10937 | << FixItHint::CreateReplacement(OpRange, SuggestedExpr); | ||||

10938 | } else if (LeftSideValue == 10) { | ||||

10939 | std::string SuggestedValue = "1" + std::string(RightSideIntValue, '0'); | ||||

10940 | S.Diag(Loc, diag::warn_xor_used_as_pow_base_ten) | ||||

10941 | << ExprStr << XorValue.toString(10, true) << SuggestedValue | ||||

10942 | << FixItHint::CreateReplacement(OpRange, SuggestedValue); | ||||

10943 | } | ||||

10944 | | ||||

10945 | S.Diag(Loc, diag::note_xor_used_as_pow_silence) | ||||

10946 | << ("(" + LHSStr + ") ^ " + RHSStr); | ||||

10947 | } | ||||

10948 | } | ||||

10949 | | ||||

10893 | QualType Sema::CheckVectorLogicalOperands(ExprResult &LHS, ExprResult &RHS, | 10950 | QualType Sema::CheckVectorLogicalOperands(ExprResult &LHS, ExprResult &RHS, | ||

Not Done ReplyI would rather see this whole section as a three-liner: // Do not diagnose binary, octal, or hexadecimal literals. if (StringRef(LHSStr).startswith("0") || StringRef(RHSStr).startswith("0")) return; Quuxplusone: I would rather see this whole section as a three-liner:
// Do not diagnose binary, octal… | |||||

10894 | SourceLocation Loc) { | 10951 | SourceLocation Loc) { | ||

Done ReplyThis ignores jfb: This ignores `0B`. | |||||

10895 | // Ensure that either both operands are of the same vector type, or | 10952 | // Ensure that either both operands are of the same vector type, or | ||

10896 | // one operand is of a vector type and the other is of its element type. | 10953 | // one operand is of a vector type and the other is of its element type. | ||

10897 | QualType vType = CheckVectorOperands(LHS, RHS, Loc, false, | 10954 | QualType vType = CheckVectorOperands(LHS, RHS, Loc, false, | ||

Done ReplyThis ignores jfb: This ignores `0X`. | |||||

10898 | /*AllowBothBool*/true, | 10955 | /*AllowBothBool*/true, | ||

10899 | /*AllowBoolConversions*/false); | 10956 | /*AllowBoolConversions*/false); | ||

10900 | if (vType.isNull()) | 10957 | if (vType.isNull()) | ||

10901 | return InvalidOperands(Loc, LHS, RHS); | 10958 | return InvalidOperands(Loc, LHS, RHS); | ||

10902 | if (getLangOpts().OpenCL && getLangOpts().OpenCLVersion < 120 && | 10959 | if (getLangOpts().OpenCL && getLangOpts().OpenCLVersion < 120 && | ||

Done ReplyI don't understand why parenthesizing one argument should silence the warning. Wouldn't it be more natural to suggest converting both arguments to hexadecimal or binary? That is, convert Quuxplusone: I don't understand why parenthesizing one argument should silence the warning. Wouldn't it be… | |||||

10903 | !getLangOpts().OpenCLCPlusPlus && vType->hasFloatingRepresentation()) | 10960 | !getLangOpts().OpenCLCPlusPlus && vType->hasFloatingRepresentation()) | ||

10904 | return InvalidOperands(Loc, LHS, RHS); | 10961 | return InvalidOperands(Loc, LHS, RHS); | ||

Done ReplyThe above seems to mishandle user-defined literals. Unless the UDL contains jfb: The above seems to mishandle user-defined literals. Unless the UDL contains `0x` or `0b` in… | |||||

Done ReplyMight as well combine these. aaron.ballman: Might as well combine these. | |||||

10905 | // FIXME: The check for C++ here is for GCC compatibility. GCC rejects the | 10962 | // FIXME: The check for C++ here is for GCC compatibility. GCC rejects the | ||

10906 | // usage of the logical operators && and || with vectors in C. This | 10963 | // usage of the logical operators && and || with vectors in C. This | ||

Done ReplyHere and on line 10971, I suggest whitespace around the Quuxplusone: Here and on line 10971, I suggest whitespace around the `<<` (in the suggested expression). | |||||

10907 | // check could be notionally dropped. | 10964 | // check could be notionally dropped. | ||

10908 | if (!getLangOpts().CPlusPlus && | 10965 | if (!getLangOpts().CPlusPlus && | ||

10909 | !(isa<ExtVectorType>(vType->getAs<VectorType>()))) | 10966 | !(isa<ExtVectorType>(vType->getAs<VectorType>()))) | ||

10910 | return InvalidLogicalVectorOperands(Loc, LHS, RHS); | 10967 | return InvalidLogicalVectorOperands(Loc, LHS, RHS); | ||

10911 | 10968 | | |||

10912 | return GetSignedVectorType(LHS.get()->getType()); | 10969 | return GetSignedVectorType(LHS.get()->getType()); | ||

10913 | } | 10970 | } | ||

10914 | 10971 | | |||

10915 | inline QualType Sema::CheckBitwiseOperands(ExprResult &LHS, ExprResult &RHS, | 10972 | inline QualType Sema::CheckBitwiseOperands(ExprResult &LHS, ExprResult &RHS, | ||

10916 | SourceLocation Loc, | 10973 | SourceLocation Loc, | ||

10917 | BinaryOperatorKind Opc) { | 10974 | BinaryOperatorKind Opc) { | ||

10918 | checkArithmeticNull(*this, LHS, RHS, Loc, /*isCompare=*/false); | 10975 | checkArithmeticNull(*this, LHS, RHS, Loc, /*isCompare=*/false); | ||

10919 | 10976 | | |||

10920 | bool IsCompAssign = | 10977 | bool IsCompAssign = | ||

10921 | Opc == BO_AndAssign || Opc == BO_OrAssign || Opc == BO_XorAssign; | 10978 | Opc == BO_AndAssign || Opc == BO_OrAssign || Opc == BO_XorAssign; | ||

10922 | 10979 | | |||

10923 | if (LHS.get()->getType()->isVectorType() || | 10980 | if (LHS.get()->getType()->isVectorType() || | ||

10924 | RHS.get()->getType()->isVectorType()) { | 10981 | RHS.get()->getType()->isVectorType()) { | ||

10925 | if (LHS.get()->getType()->hasIntegerRepresentation() && | 10982 | if (LHS.get()->getType()->hasIntegerRepresentation() && | ||

Not Done ReplySuppressing the warning specifically on Quuxplusone: Suppressing the warning specifically on `10 ^ 0` (which means `10`) seems like an anti-feature. | |||||

10926 | RHS.get()->getType()->hasIntegerRepresentation()) | 10983 | RHS.get()->getType()->hasIntegerRepresentation()) | ||

Not Done ReplyDo you currently warn on, let's say, Quuxplusone: Do you currently warn on, let's say, `2uLL ^ 7`? and if so, do we care that the suggested… | |||||

Done ReplyWe check if bidwith is same.. Bail out if not. xbolva00: We check if bidwith is same.. Bail out if not. | |||||

10927 | return CheckVectorOperands(LHS, RHS, Loc, IsCompAssign, | 10984 | return CheckVectorOperands(LHS, RHS, Loc, IsCompAssign, | ||

10928 | /*AllowBothBool*/true, | 10985 | /*AllowBothBool*/true, | ||

10929 | /*AllowBoolConversions*/getLangOpts().ZVector); | 10986 | /*AllowBoolConversions*/getLangOpts().ZVector); | ||

10930 | return InvalidOperands(Loc, LHS, RHS); | 10987 | return InvalidOperands(Loc, LHS, RHS); | ||

10931 | } | 10988 | } | ||

Not Done ReplyI suggest at least one unit test case that involves Quuxplusone: I suggest at least one unit test case that involves `10 ^ 100`, and considering the user… | |||||

10932 | 10989 | | |||

10933 | if (Opc == BO_And) | 10990 | if (Opc == BO_And) | ||

10934 | diagnoseLogicalNotOnLHSofCheck(*this, LHS, RHS, Loc, Opc); | 10991 | diagnoseLogicalNotOnLHSofCheck(*this, LHS, RHS, Loc, Opc); | ||

10935 | 10992 | | |||

10993 | if (Opc == BO_Xor) | ||||

10994 | diagnoseXorMisusedAsPow(*this, LHS, RHS, Loc); | ||||

10995 | | ||||

10936 | ExprResult LHSResult = LHS, RHSResult = RHS; | 10996 | ExprResult LHSResult = LHS, RHSResult = RHS; | ||

10937 | QualType compType = UsualArithmeticConversions(LHSResult, RHSResult, | 10997 | QualType compType = UsualArithmeticConversions(LHSResult, RHSResult, | ||

10938 | IsCompAssign); | 10998 | IsCompAssign); | ||

10939 | if (LHSResult.isInvalid() || RHSResult.isInvalid()) | 10999 | if (LHSResult.isInvalid() || RHSResult.isInvalid()) | ||

10940 | return QualType(); | 11000 | return QualType(); | ||

10941 | LHS = LHSResult.get(); | 11001 | LHS = LHSResult.get(); | ||

10942 | RHS = RHSResult.get(); | 11002 | RHS = RHSResult.get(); | ||

10943 | 11003 | | |||

▲ Show 20 Lines • Show All 6183 Lines • Show Last 20 Lines |

Missing a full stop at the end of the comment. Same comment applies elsewhere.