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

[Arthur O'Dwyer via cfe-commits]

Quuxplusone added a subscriber: Quuxplusone.
Quuxplusone added a comment.

It seems like this proposed diagnostic and fixit, statistically speaking, is *never* correct.
In the cases where there is a code issue to be corrected, the diagnosable issue really seems to involve dataflow analysis:

"Here is a variable of integral type (not bool), which over its lifetime assumes only the values `0` and `1`. This variable should be declared as type `bool`."
"Here is a function returning integral type (not bool), which returns only the values `0` and `1`. This function should be declared as returning type `bool`."
"Here is a parameter of integral type (not bool), which (via whole-program analysis) assumes only the values `0` and `1`. This parameter should be declared as type `bool`."


================
Comment at: lib/AST/DeclPrinter.cpp:1016
@@ -1015,3 +1015,3 @@
   Out << "<";
-  unsigned First = true;
+  unsigned First = 1;
   for (auto *Param : *Params) {
----------------
This is clearly the wrong correction, don't you think? Should be s/unsigned/bool/, not s/true/1/.

================
Comment at: lib/AST/DeclarationName.cpp:104
@@ -105,1 +103,3 @@
+      case -1: return 1;
+      case 1: return 0;
       default: break;
----------------
This seems like it might be a real bug: somebody in the past cut-and-pasting the implementation of operator<() into this compare() function. I suspect (but please don't trust me) that the proper fix is s/true/-1/ s/false/1/.

================
Comment at: lib/Bitcode/Writer/BitWriter.cpp:40
@@ -39,3 +39,3 @@
 int LLVMWriteBitcodeToFileHandle(LLVMModuleRef M, int FileHandle) {
-  return LLVMWriteBitcodeToFD(M, FileHandle, true, false);
+  return LLVMWriteBitcodeToFD(M, FileHandle, 1, 0);
 }
----------------
This also seems like the wrong correction.

================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:1332
@@ -1331,3 +1331,3 @@
   assert(N->isDistinct() && "Expected distinct compile units");
-  Record.push_back(/* IsDistinct */ true);
+  Record.push_back(/* IsDistinct */ 1);
   Record.push_back(N->getSourceLanguage());
----------------
This seems like a "correct" but unwanted adjustment. *Maybe* there's a case for `static_cast<uint64_t>(true)`... but personally I'd leave the `true` alone.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:593
@@ -592,3 +592,3 @@
   switch (Op.getOpcode()) {
-  default: return false;
+  default: return 0;
   case ISD::ConstantFP:
----------------
Here is the first clearly beneficial correction I've noticed.

================
Comment at: lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp:1562
@@ -1563,1 +1561,3 @@
+    IsBottomUp = 1,
+    HasReadyFilter = 0
   };
----------------
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.

================
Comment at: lib/Driver/Tools.cpp:2022
@@ -2021,3 +2021,3 @@
 
-    if (OptionIter->second == true) {
+    if (OptionIter->second == 1) {
       // Duplicate option specified.
----------------
Shouldn't the original code already have triggered some warning about explicit comparison with a boolean value?

    if (OptionIter->second) {

doesn't have literally the same semantics, but it's clearly IMHO what was intended.
Also, if `decltype(llvm::StringMap<bool>::iterator{}->second)` is not `bool`, something weird is going on.

================
Comment at: lib/IR/Core.cpp:227
@@ -226,3 +226,3 @@
     *ErrorMessage = strdup(EC.message().c_str());
-    return true;
+    return 1;
   }
----------------
This correction is wrong; either `true` should be accepted as a valid value for `LLVMBool`, or `LLVMBool` should be replaced with `bool`.


http://reviews.llvm.org/D19105