Page MenuHomePhabricator

Changes in clang after running http://reviews.llvm.org/D18821
AbandonedPublic

Authored by Prazek on Apr 14 2016, 4:10 AM.

Details

Summary

I am not done with analysis, and I don't want to push it to repo as it is, because in many cases when programmer wrote true/false he wanted to have bool instead of int.

Some nice example:
unsigned IsFilled : 1;

/// Simplified kind of \c CommentDecl, see \c DeclKind enum.
unsigned Kind : 3;

/// Is \c CommentDecl a template declaration.
unsigned TemplateKind : 2;

/// Is \c CommentDecl an ObjCMethodDecl.
unsigned IsObjCMethod : 1;

/// Is \c CommentDecl a non-static member function of C++ class or
/// instance method of ObjC class.
/// Can be true only if \c IsFunctionDecl is true.
unsigned IsInstanceMethod : 1;

/// Is \c CommentDecl a static member function of C++ class or
/// class method of ObjC class.
/// Can be true only if \c IsFunctionDecl is true.
unsigned IsClassMethod : 1;

I want to replace all unsigned that are 1 bits with bool.

Diff Detail

Event Timeline

Prazek updated this revision to Diff 53687.Apr 14 2016, 4:10 AM
Prazek retitled this revision from to Changes in clang after running http://reviews.llvm.org/D18821.
Prazek updated this object.
Prazek added a reviewer: alexfh.
Prazek added a subscriber: cfe-commits.

I want to replace all unsigned that are 1 bits with bool.

MSVC only packs bitfields of the same type together, so doing that change would make clang use much more memory on Windows.

I want to replace all unsigned that are 1 bits with bool.

MSVC only packs bitfields of the same type together, so doing that change would make clang use much more memory on Windows.

Seriously MSVC? Seriously?
I guess I will have add something to my check to ignore bitfields of 1.

alexfh edited edge metadata.Apr 15 2016, 5:07 AM

Ignoring 1-bit wide bitfields seems like a reasonable thing to do for the check. Most of the changes I see here are making the code less idiomatic, I'd say.

alexfh requested changes to this revision.Apr 15 2016, 5:07 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.Apr 15 2016, 5:07 AM
Prazek updated this revision to Diff 56250.May 5 2016, 2:44 AM
Prazek edited edge metadata.

It seems that is doing it's work right now.

I will have to change some places to post it to llvm like:

  • changing functions return type to bool
  • removng comparasion with bool for bools

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."

lib/AST/DeclPrinter.cpp
1016

This is clearly the wrong correction, don't you think? Should be s/unsigned/bool/, not s/true/1/.

lib/AST/DeclarationName.cpp
104

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/.

lib/Bitcode/Writer/BitWriter.cpp
40

This also seems like the wrong correction.

lib/Bitcode/Writer/BitcodeWriter.cpp
1332

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.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
594

Here is the first clearly beneficial correction I've noticed.

lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp
1562

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.

lib/Driver/Tools.cpp
2022 ↗(On Diff #56250)

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.

lib/IR/Core.cpp
222

This correction is wrong; either true should be accepted as a valid value for LLVMBool, or LLVMBool should be replaced with bool.

jfb added inline comments.May 5 2016, 12:36 PM
lib/Transforms/IPO/MergeFunctions.cpp
147

This change should go to ValueMap, I think it should be moved to an enum class, and all uses of ValueMap should be changed accordingly. This is meant to act as a bool IMO, it's odd to make it otherwise.

Prazek added a comment.May 6 2016, 6:52 AM

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."

You are right, if someone is using true/false, then he probably mean it, and he is just assigning it to wrong type. But I don't see a point spending too much time to write something that would be smart enough to fix most of the issues as we would like. I think having this fixit is good for now and I would leave it like this.
From my usage, I like when clang-tidy touch some files and changes it somewhow, so it is much simpler to check it in git than to look at the warnings.

lib/Driver/Tools.cpp
2022 ↗(On Diff #56250)

te misc-simplify-boolean-expression is to solve this problem. I will check if this code make sense.

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

clang-tidy/readability/ImplicitBoolCastCheck.cpp
253 ↗(On Diff #56250)

Seems like a bug. CXXBoolLiteralExpr::getValue() returns bool.

lib/AsmParser/LLParser.cpp
4868

Looks like this should return InstResult and the return values should be replaced by proper enumerators.

lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp
1562

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.

lib/IR/Core.cpp
222

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.

lib/Lex/PPMacroExpansion.cpp
1689

This actually seems like a useful replacement ;)

lib/MC/MCContext.cpp
314

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.

lib/Sema/SemaOpenMP.cpp
4487

Instead, the parameter type should be changed to bool.

lib/Serialization/ASTReader.cpp
5453

That's wrong. Instead, Record[Idx++] should be changed to Record[Idx++] != 0.

lib/Target/Hexagon/MCTargetDesc/HexagonMCShuffler.cpp
194 ↗(On Diff #56250)

This is wrong.

alexfh requested changes to this revision.May 11 2016, 2:16 AM
alexfh edited edge metadata.

The bottom line is that the fixit in its current form is not particularly useful in most cases. We should try to detect some cases where the replacement is unlikely to be useful, e.g. the enum { FollowRAUW = false }; pattern, returning a bool from a function that is declared to return a typedef to an integral type that contains bool in its name (e.g. LLVMBool), and maybe some other cases.

This revision now requires changes to proceed.May 11 2016, 2:16 AM

returning a bool from a function that is declared to return a typedef to an integral type that contains bool in its name (e.g. LLVMBool), and maybe some other cases.

Isn't it LLVMBool issue?

I won't have enough time to fix it, so I would prefer to comment fixit and only push warnings.

Prazek added inline comments.May 11 2016, 8:28 AM
lib/Target/Hexagon/MCTargetDesc/HexagonMCShuffler.cpp
194 ↗(On Diff #56250)

Of course it is, but the prevoius code was also creepy :P

returning a bool from a function that is declared to return a typedef to an integral type that contains bool in its name (e.g. LLVMBool), and maybe some other cases.

Isn't it LLVMBool issue?

I won't have enough time to fix it, so I would prefer to comment fixit and only push warnings.

Whether we disable fixits or not, there seem to be too many false positives for the check to be useful. We should fix at least cases like bool b; ... if (b == true) ... and bool b; ... b ^= true;.

Prazek updated this revision to Diff 58313.May 24 2016, 1:57 PM
Prazek edited edge metadata.

Fixed many stuff.

Note that I won't push this patch mainly because of LLVMBool stuff and some other small issues. I don't think there can be done anything more for this check.

deadalnix requested changes to this revision.May 25 2016, 3:11 AM
deadalnix edited edge metadata.
deadalnix added inline comments.
include/llvm-c/Core.h
604

Please keep LLVMBool in the C API. ABI change are a NO NO.

This revision now requires changes to proceed.May 25 2016, 3:11 AM
Prazek added inline comments.May 25 2016, 3:18 AM
include/llvm-c/Core.h
604

As I said in my comment, I won't commit most of the changes. This review is only to show what check has found.

alexfh added a comment.Jun 3 2016, 6:30 PM

Most of the fixits are still useless, but I didn't notice any false positives (i.e. all warnings would be useful to some degree). So I suggest disabling the fixes completely at least for now.

include/llvm-c/Core.h
604

From what I see in this patch, changing the function type to bool not a useful fix, and certainly a dangerous one, since 1. ABI changes, 2. The check can't see all the users of the function.

lib/AST/Decl.cpp
173

Apparently, the check should filter out template instantiations.

lib/CodeGen/CGOpenMPRuntime.cpp
1934

Looks like the parameter type should be changed to bool instead.

lib/CodeGen/SplitKit.h
298

Is it an automated fix?

lib/Target/ARM/ARMConstantIslandPass.cpp
1643

In this case, changing the type would be better.

utils/TableGen/CTagsEmitter.cpp
36

One of the few cases where changing the function return type makes sense.

Prazek abandoned this revision.Feb 13 2017, 2:00 AM
Prazek added inline comments.
lib/CodeGen/SplitKit.h
298

No, this is the only one that I'have made by hand, so it won't change some calls on this type