[PATCH] D19105: Changes in clang after running http://reviews.llvm.org/D18821

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

Too much noise here as well. Particularly, `someBool == true` should not be changed to `someBool == 1`. Please fix the check and update the results.


================
Comment at: clang-tidy/readability/ImplicitBoolCastCheck.cpp:253
@@ -252,3 +252,3 @@
        DestinationType->isMemberPointerType()) &&
-      BoolLiteralExpression->getValue() == false) {
+      BoolLiteralExpression->getValue() == 0) {
     return "0";
----------------
Seems like a bug. `CXXBoolLiteralExpr::getValue()` returns `bool`.

================
Comment at: lib/AsmParser/LLParser.cpp:4868
@@ -4867,3 +4867,3 @@
 ///
 int LLParser::ParseInstruction(Instruction *&Inst, BasicBlock *BB,
                                PerFunctionState &PFS) {
----------------
Looks like this should return `InstResult` and the return values should be replaced by proper enumerators.

================
Comment at: lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp:1562
@@ -1563,1 +1561,3 @@
+    IsBottomUp = 1,
+    HasReadyFilter = 0
   };
----------------
Quuxplusone wrote:
> If I understand correctly, the idiomatic C++11 way to write this code would be
> 
>     static constexpr bool IsBottomUp = true;
>     static constexpr bool HasReadyFilter = false;
> 
> I think the proposed correction is worse than the original, because it makes this look like an integer enumeration.
I agree that changing this to integers is a regression. However, we can't yet use `constexpr`, since VS2013 doesn't support it. Looks like this file just needs to be reverted.

================
Comment at: lib/IR/Core.cpp:227
@@ -226,3 +226,3 @@
     *ErrorMessage = strdup(EC.message().c_str());
-    return true;
+    return 1;
   }
----------------
Quuxplusone wrote:
> This correction is wrong; either `true` should be accepted as a valid value for `LLVMBool`, or `LLVMBool` should be replaced with `bool`.
This function is a part of C API, so we can't use `bool` here, and `LLVMBool` is the right choice. However, it might make sense to add `LLVM_TRUE` and `LLVM_FALSE` constants/macros to `include/llvm-c/Types.h` to more clearly specify possible values for the `LLVMBool` type.

================
Comment at: lib/Lex/PPMacroExpansion.cpp:1689
@@ -1688,3 +1688,3 @@
         if (!II)
-          return false;
+          return 0;
         else if (II->getBuiltinID() != 0)
----------------
This actually seems like a useful replacement ;)

================
Comment at: lib/MC/MCContext.cpp:314
@@ -313,3 +313,3 @@
       MCSectionELF(I->getKey(), Type, Flags, SectionKind::getReadOnly(),
-                   EntrySize, Group, true, nullptr, Associated);
+                   EntrySize, Group, 1, nullptr, Associated);
 }
----------------
That one is interesting: passing `true` (or `1`) to `unsigned UniqueID` seems like a very weird thing to do. Please at least add a `/*UniqueID=*/` comment before the argument to make this weirdness more explicit.

================
Comment at: lib/Sema/SemaOpenMP.cpp:4487
@@ -4486,3 +4486,3 @@
   // OK to convert to signed, because new type has more bits than old.
-  QualType NewType = C.getIntTypeForBitwidth(Bits, /* Signed */ true);
+  QualType NewType = C.getIntTypeForBitwidth(Bits, /* Signed */ 1);
   return SemaRef.PerformImplicitConversion(E, NewType, Sema::AA_Converting,
----------------
Instead, the parameter type should be changed to bool.

================
Comment at: lib/Serialization/ASTReader.cpp:5453
@@ -5452,3 +5452,3 @@
     AutoTypeKeyword Keyword = (AutoTypeKeyword)Record[Idx++];
-    bool IsDependent = Deduced.isNull() ? Record[Idx++] : false;
+    bool IsDependent = Deduced.isNull() ? Record[Idx++] : 0;
     return Context.getAutoType(Deduced, Keyword, IsDependent);
----------------
That's wrong. Instead, `Record[Idx++]` should be changed to `Record[Idx++] != 0`.

================
Comment at: lib/Target/Hexagon/MCTargetDesc/HexagonMCShuffler.cpp:194
@@ -193,3 +193,3 @@
 
-  if (doneShuffling == false) {
+  if (doneShuffling == 0) {
     HexagonMCShuffler MCS(MCII, STI, MCB);
----------------
This is wrong.


http://reviews.llvm.org/D19105