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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
