Clang-tidy: add braces after control statements

[fabiocos]

This issue is meant to keep track of the proposed update to clang-tidy instructions in #21478 ,readability-braces-around-statements, that has been kept on hold so far due to problems in the application, preventing a safe use of this option.

The update proposed by @fwyzard is meant to add braces after control statements, e.g. change

if (something)
  do_something();

to

if (something) {
  do_something();
}

As mentioned by @smuzaffar in https://github.com/cms-sw/cmssw/pull/21478#issuecomment-495148294 there are long standing bugs that sometimes generate odd code:

For statements which ends with }; e.g

if (condition) return {}; it generates code like

if (condition) { return {}}; In some cases it also adds extra {} for classes/struct

diff --git a/OnlineDB/EcalCondDB/interface/LMFLmrSubIOV.h b/OnlineDB/EcalCondDB/interface/LMFLmrSubIOV.h
index 78fc1a87de4..15aa2925220 100644
--- a/OnlineDB/EcalCondDB/interface/LMFLmrSubIOV.h
+++ b/OnlineDB/EcalCondDB/interface/LMFLmrSubIOV.h
@@ -10,7 +10,8 @@
 #include "OnlineDB/EcalCondDB/interface/LMFUnique.h"
 #include "OnlineDB/EcalCondDB/interface/LMFIOV.h"

-class LMFLmrSubIOV : public LMFUnique {
+class LMFLmrSubIOV { 
+}: public LMFUnique {
  public:
   friend class EcalCondDBInterface;

diff --git a/OnlineDB/EcalCondDB/interface/LMFSextuple.h b/OnlineDB/EcalCondDB/interface/LMFSextuple.h
index 6559c9f769c..1573587ec4a 100644
--- a/OnlineDB/EcalCondDB/interface/LMFSextuple.h
+++ b/OnlineDB/EcalCondDB/interface/LMFSextuple.h
@@ -10,7 +10,8 @@
 /**
  *   sextuple of t1, t2, t3, p1, p2, p3
  */
-class LMFSextuple {
+class LMFSextuple { 
+}{
  public:
   LMFSextuple() {
     for (int i = 0; i < 3; i++) {

The problem is reproducible with a test like

scram p CMSSW_10_6_X_2019-04-29-2300
cd CMSSW_10_6_X_2019-04-29-2300/src
cmsenv
git cms-merge-topic -u 21478
git cms-addpkg OnlineDB/EcalCondDB
scram b llvm-ccdb
clang-tidy -fix -header-filter "$CMSSW_BASE/src/.*" OnlineDB/EcalCondDB/src/LMFCorrCoefDat.cc

[cmsbuild]

A new Issue was created by @fabiocos Fabio Cossutti.

@davidlange6, @Dr15Jones, @smuzaffar, @fabiocos, @kpedro88 can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

[fabiocos]

assign core

[cmsbuild]

New categories assigned: core

@Dr15Jones,@smuzaffar you have been requested to review this Pull request/Issue and eventually sign? Thanks

[smuzaffar]

I recheck this for LLVM and noticed that

1. One issue [a] reported there https://github.com/cms-sw/cmssw/pull/21478#issuecomment-487903598 can be avoided by setting readability-braces-around-statements.ShortStatementLines to 1.

2. The other issue [b] still exists and it generate invalid code

   --- a/OnlineDB/EcalCondDB/src/Foo.cc
   +++ b/OnlineDB/EcalCondDB/src/Foo.cc
   @@ -16,6 +16,7 @@ class BAR: public FOO
   BAR getBar(FOO *p)
   {
   BAR b;
   -  if (p->get()>0) return b;
   -  else return {};
   +  if (p->get()>0) { return b;
   +  } else { return {}
   +};
   }

The bug report https://bugs.llvm.org/show_bug.cgi?id=25970 is still open.

[a]

diff --git a/OnlineDB/EcalCondDB/interface/LMFSextuple.h b/OnlineDB/EcalCondDB/interface/LMFSextuple.h
index 6559c9f769c..1573587ec4a 100644
--- a/OnlineDB/EcalCondDB/interface/LMFSextuple.h
+++ b/OnlineDB/EcalCondDB/interface/LMFSextuple.h
@@ -10,7 +10,8 @@
 /**
  *   sextuple of t1, t2, t3, p1, p2, p3
  */
-class LMFSextuple {
+class LMFSextuple { 
+}{
  public:
   LMFSextuple() {
     for (int i = 0; i < 3; i++) {

[b]

- if (condition) return {};
+ if (condition) { return {}};

[fwyzard]

Hi Shahzad, to clarify - the issue with [b] is that it transforms

if (condition) return {};

into

if (condition) { return {}};

while the right transofmation would be

if (condition) { return {}; }

?

[smuzaffar]

yes that is correct @fwyzard

[smuzaffar]

there is a fix in pipeline https://reviews.llvm.org/D75813 . We will try again once this is integrated

[fabiocos]

@smuzaffar so apparently this is the only problem still keeping on hold the migration? But how often do we have it? A quick browsing does not show such kind of instances, I might be missing them. Of course in case of a migration better to be sure of the correct behaviour, something might be missed while approving, I agree...

[fabiocos]

@smuzaffar is there any news about this? Is this request still on the radar?

[silviodonato]

I see that the fix has been merged in LLVM main, https://reviews.llvm.org/rG0becc4d721d0036e2e38d581bc487e27f78eb8a9 but unfortunately it seems that it missed LLVM 12.0.

In other words, I see the commit https://github.com/llvm/llvm-project/commit/0becc4d721d0036e2e38d581bc487e27f78eb8a9 is included in https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements.cpp but it is not included in https://github.com/llvm/llvm-project/blob/llvmorg-12.0.0/clang-tools-extra/test/clang-tidy/checkers/readability-braces-around-statements.cpp

[fabiocos]

@silviodonato ok, let's wait fir next round, just please keep this in mind

[smuzaffar]

looks likle this is still not fixed for llvm 12.0.1. clang-tidy from llvm 12.0.01 still generated invalid code

>which clang-tidy
.../llvm/12.0.1-8e8f71e20b63cb9088493005bc86a4a6/bin/clang-tidy

diff --git a/OnlineDB/EcalCondDB/interface/LMFLmrSubIOV.h b/OnlineDB/EcalCondDB/interface/LMFLmrSubIOV.h
index 8521bf2e970..00b5077eba6 100644
--- a/OnlineDB/EcalCondDB/interface/LMFLmrSubIOV.h
+++ b/OnlineDB/EcalCondDB/interface/LMFLmrSubIOV.h
@@ -10,7 +10,8 @@
 #include "OnlineDB/EcalCondDB/interface/LMFUnique.h"
 #include "OnlineDB/EcalCondDB/interface/LMFIOV.h"

-class LMFLmrSubIOV : public LMFUnique {
+class LMFLmrSubIOV {
+}: public LMFUnique {
 public:
   friend class EcalCondDBInterface;

diff --git a/OnlineDB/EcalCondDB/interface/LMFSextuple.h b/OnlineDB/EcalCondDB/interface/LMFSextuple.h
index db955236266..159cd6a9973 100644
--- a/OnlineDB/EcalCondDB/interface/LMFSextuple.h
+++ b/OnlineDB/EcalCondDB/interface/LMFSextuple.h
@@ -10,7 +10,8 @@
 /**
  *   sextuple of t1, t2, t3, p1, p2, p3
  */
-class LMFSextuple {
+class LMFSextuple {
+}{
 public:
   LMFSextuple() {
     for (int i = 0; i < 3; i++) {