Add braces after control statements #21478
Conversation
|
@cmsbuild, please test |
|
The code-checks are being triggered in jenkins. |
|
+code-checks |
|
The tests are being triggered in jenkins. |
|
A new Pull Request was created by @fwyzard (Andrea Bocci) for master. It involves the following packages: .clang-tidy The following packages do not have a category, yet: .clang-tidy @cmsbuild can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
cmsbuild
added
tests-started
core-pending
and removed
tests-pending
new-package-pending
labels
|
Pull request #21478 was updated. @smuzaffar, @Dr15Jones can you please check and sign again. |
|
+1 |
cmsbuild
added
core-approved
fully-signed
and removed
core-pending
pending-signatures
labels
|
This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar (and backports should be raised in the release meeting by the corresponding L2) |
|
I tried applying this -
the actual changes look like
- if (triggerResults_ == nullptr) throwTriggerResultsMissing();
+ if (triggerResults_ == nullptr) { throwTriggerResultsMissing(); }
- if (!localconfservice.isAvailable())
- throw cms::Exception("TrivialFileCatalog", "edm::SiteLocalConfigService is not available");
+ if (!localconfservice.isAvailable()) {
+ throw cms::Exception("TrivialFileCatalog", "edm::SiteLocalConfigService is not available"); }
… On Nov 28, 2017, at 11:49 AM, Andrea Bocci ***@***.***> wrote:
This should add braces after control statements, e.g. change
if (something)
do_something();
to
if (something) {
do_something();
}
You can view, comment on, or merge this pull request online at:
#21478
Commit Summary
• Add braces after control statements
File Changes
• M .clang-tidy (1)
Patch Links:
• https://github.com/cms-sw/cmssw/pull/21478.patch
• https://github.com/cms-sw/cmssw/pull/21478.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
I'd say it was an improvement. |
|
Maybe it expects clang-format to take care of the newlines etc. ? |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
The tests are being triggered in jenkins. |
|
-1 it generates code like In some cases it also adds extra {} for classes/struct you can reproduce it by running |
cmsbuild
added
core-rejected
pending-signatures
and removed
core-approved
fully-signed
labels
|
Indeed, I can reproduce it also with a very simple, stand alone test case. Looks like this option is not ready for prime time... can anyone check if the clang 9 nightly hhas better luck, and/or submit a but report to clang ? |
|
On Apr 30, 2019, at 5:34 AM, Malik Shahzad Muzaffar ***@***.***> wrote:
-1
@fwyzard, for some cases this check is not generating valid code. For statements which ends with }; e.g
if (condition) return {};
does this syntax make sense?
it generates code like
if (condition) { return {}};
not sure what else it should do? perhaps the issue is with the original code...
In some cases it also adds extra {} for classes/struct
these are more interesting... the feature this seems undocumented, unfortunately
…
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++) {
you can reproduce it by running
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
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
yes, for example when returning a struct by value.
I think it should have become |
|
On Apr 30, 2019, at 10:56 AM, Matti Kortelainen ***@***.***> wrote:
if (condition) return {};
does this syntax make sense?
return {}; is a short hand for the default constructor of the return type of the function.
ah - didn't know that - thanks - is then {return {}} not valid and expected syntax then?
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@fwyzard @smuzaffar from the latest checks it looks like this option is not ready for production, at least until a fix is provided for the problematic cases observed. What should we do with this PR? Close it and may be open an issue to track the planned update, verifying whether there is a fix in clang? |
|
We can do whatever you think makes the most sense. |
|
hold pending a solution in clang-tidy for the issue above mentioned |
|
Pull request has been put on hold by @fabiocos |
|
There are quite a few bugs opens for this check the one I mentioned #21478 (comment) is open since DEC 2015 ( https://bugs.llvm.org/show_bug.cgi?id=25970 ). |
|
@smuzaffar yes, this hold risks to last quite long... :-( |
|
Tracked by #27082 . |
This should add braces after control statements, e.g. change
to