Clang-tidy fix is removing business logic.

[Quuxplusone]

Bugzilla Link	PR38240
Status	NEW
Importance	P normal
Reported by	Tom Schultz (tom.schultz@km.kongsberg.com)
Reported on	2018-07-20 00:08:03 -0700
Last modified on	2018-08-28 12:55:56 -0700
Version	unspecified
Hardware	PC Windows NT
CC	alexfh@google.com, djasper@google.com, klimek@google.com, legalize@xmission.com, tom.schultz@km.kongsberg.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

OS windows 10 (64)
C:\Program Files\LLVM\bin>clang-tidy -version
LLVM (http://llvm.org/):
  LLVM version 5.0.0
  Optimized build.
  Default target: x86_64-pc-windows-msvc
  Host CPU: skylake

Ran clang-tidy fix on some files in my project, it seems like the [readability-
simplify-boolean-expr] is a bit to aggressive in removing code.

Here is the LOG:
10: C:\Repository\###\BaseInstance.cpp
2860 warnings and 1 error generated.
Error while processing C:\Repository\###\BaseInstance.cpp.
C:\Repository\###\..\..\source\code\include\###/lib/stdafx.h(26): error [clang-
diagnostic-error]: '###/logging/facade/logger.h' file not found
#include <###core/logging/facade/logger.h>
         ^
C:\Repository\###\BaseInstance.cpp(27): warning [modernize-use-equals-default]:
use '= default' to define a trivial default constructor
BaseInstance::BaseInstance()
              ^
C:\Repository\###\BaseInstance.cpp(39): warning [google-readability-todo]:
missing username/bug in TODO
        //TODO do stuff here.
        ^~~~~~~~~~~~~~~~~~~~~
        // TODO(###): do stuff here.
C:\Repository\###\BaseInstance.cpp(39): message: FIX-IT applied suggested code
changes
C:\Repository\###\BaseInstance.cpp(40): warning [readability-simplify-boolean-
expr]: redundant boolean literal in conditional return statement
        return true;
               ^
C:\Repository\###\BaseInstance.cpp(84): warning [readability-simplify-boolean-
expr]: redundant boolean literal in conditional return statement
        return true;
~~~~~~~~~~~~~~~^~~~~
C:\Repository\###\BaseInstance.cpp(79): message: FIX-IT applied suggested code
changes
    if (it != m_Storage.end())
    ^
clang-tidy applied 2 of 2 suggested fixes.

And here is the DIFF of this file:
--- a/source/code/src/KcsAggregator-Lib/BaseInstance.cpp
+++ b/source/code/src/KcsAggregator-Lib/BaseInstance.cpp
@@ -36,7 +36,7 @@ bool BaseInstance::RegisterDataValueBinding(
     const auto it = m_Storage.find(elementId);
     if (it != m_Storage.end())
     {
-        //TODO do stuff here.
+        // TODO(###): do stuff here.
         return true;
     }
     return false;
@@ -76,14 +76,7 @@ bool BaseInstance::GetDataValue(
 {
     const ElementId subId (0, TupleNumeric(0, elementId.GetSubId()));
     const auto it = m_Storage.find(subId);
-    if (it != m_Storage.end())
-    {
-        value = DataValue(it->second,
-                            AttributeType::Value,
-                            SystemTime::GetSystemTime());
-        return true;
-    }
-    return false;
+    return ;
 }

The same thing happened i many files, and seems to be removing whole if clauses.
@@ -683,13 +684,7 @@ bool ConfigurationReader::BuildGraph()
     }
     while (false == m_BrowseGraphHelper.GetResult(requestId, response));

-    if (IsGood(response.GetStatusCode()))
-    {
-        response.GetGraph(m_Graph);
-        return true;
-    }
-
-    return false;
+    return ;
 }

[Quuxplusone]

Note the compilation error:

C:\Repository###....\source\code\include###/lib/stdafx.h(26): error [clang-diagnostic-error]: '###/logging/facade/logger.h' file not found

It means that the AST clang-tidy operates on is incomplete / invalid, which may result in many checks producing spurious warnings and bad fixes. Clang-tidy doesn't apply fixes in the presence of compilation errors by default (partly for that reason). Did you run it with -fix-errors?

[Quuxplusone]

(though it still may be reasonable to disable the check on code that doesn't compile)

[Quuxplusone]

Yes i did run this with -fix-errors, so i was expecting it to do changes in the files. I was not suspecting it would do this.

[Quuxplusone]

Can you provide an isolated case that reproduces the problem?