Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjust a few weird clang format updates #27005

Merged
merged 2 commits into from Jun 5, 2019

Conversation

perrotta
Copy link
Contributor

PR description:

See title
No changes expected

PR validation:

It compiles

@cmsbuild cmsbuild added this to the CMSSW_11_0_X milestone May 30, 2019
@perrotta
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27005/10065

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented May 30, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/601/console Started: 2019/05/30 12:54

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @perrotta for master.

It involves the following packages:

RecoLocalMuon/GEMSegment
RecoLocalTracker/SiStripRecHitConverter

@perrotta, @cmsbuild, @kpedro88, @slava77 can you please review it and eventually sign? Thanks.
@echabert, @pieterdavid, @makortel, @felicepantaleo, @yduhm, @GiacomoSguazzoni, @gbenelli, @jhgoh, @VinInn, @jshlee, @bellan, @rovere, @threus, @gpetruc, @ebrondol, @alesaggio, @mmusich this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@VinInn
Copy link
Contributor

VinInn commented May 30, 2019

Le me understand:
each time this (and other files fixed by hand) will be edited and re-committed clang-format will run again and produce similar if not identical abominations...
How are we supposed to continue in a sustainable manner?

@perrotta
Copy link
Contributor Author

Le me understand:
each time this (and other files fixed by hand) will be edited and re-committed clang-format will run again and produce similar if not identical abominations...
How are we supposed to continue in a sustainable manner?

Well, the fixes here (that I only submitted "not to forget", since I took note of them, but I don't plan to pursue systematically) won't be reverted by a possible further run of clang tidy, since I moved the comment line somewhere else. I deliberately avoided fixing other similarly weird changes which risk to get reverted in the next run.

float(
0.01))); //temp for floating point comparision...maxEta is the difference between partitions, so x1.5 to take into account non-circle geom.
return ( diff < std::max( maxETA, 0.01f));
//temp for floating point comparision...maxEta is the difference between partitions, so x1.5 to take into account non-circle geom.
Copy link
Contributor

@slava77 slava77 May 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a comment aligned with the code and before the action is done seems to be more common.

BTW, does the text even make sense?

  • temp for floating point comparision... sounds like it's justifying why float was added in the original code
  • what is this "x1.5 ... " about?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, does the text even make sense?

  • temp for floating point comparision... sounds like it's justifying why float was added in the original code
  • what is this "x1.5 ... " about?

I wondered the same, but it was not my intention to inquire about it here.
As you know, even diff here makes no se sense (as it is a difference of h1.eta() with itself. But fixing that bug was not the scope of this PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code was modified (float (0.01) -> 0.01f ) and it apparently affects the content of the comment.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@makortel
Copy link
Contributor

There is also a way to disable formatting for a region of code

// clang-format off
...
// clang-format on

http://releases.llvm.org/3.6.0/tools/clang/docs/ClangFormatStyleOptions.html#disabling-formatting-on-a-piece-of-code

@@ -96,8 +96,8 @@ void SiStripRecHitMatcher::match(const SiStripRecHit2D* monoRH,
// position of the initial and final point of the strip in local coordinates (mono det)
StripPosition stripmono = StripPosition(topol.localPosition(RPHIpointini), topol.localPosition(RPHIpointend));

if (trackdirection.mag2() <
FLT_MIN) { // in case of no track hypothesis assume a track from the origin through the center of the strip
// in case of no track hypothesis assume a track from the origin through the center of the strip
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this comment be indented by two more spaces? Or did clang-format place it here?

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-1c61cd/601/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 33
  • DQMHistoTests: Total histograms compared: 3210728
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3210392
  • DQMHistoTests: Total skipped: 334
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 32 files compared)
  • Checked 137 log files, 14 edm output root files, 33 DQM output files

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@perrotta
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27005/10075

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented May 30, 2019

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/613/console Started: 2019/05/30 18:14

@cmsbuild
Copy link
Contributor

Pull request #27005 was updated. @perrotta, @cmsbuild, @kpedro88, @slava77 can you please check and sign again.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-1c61cd/613/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 33
  • DQMHistoTests: Total histograms compared: 3210678
  • DQMHistoTests: Total failures: 1
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3210343
  • DQMHistoTests: Total skipped: 334
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 32 files compared)
  • Checked 137 log files, 14 edm output root files, 33 DQM output files

@perrotta
Copy link
Contributor Author

+1

  • Technical
  • Jenkins tests pass

@kpedro88
Copy link
Contributor

kpedro88 commented Jun 4, 2019

+upgrade

@cmsbuild
Copy link
Contributor

cmsbuild commented Jun 4, 2019

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

@fabiocos
Copy link
Contributor

fabiocos commented Jun 5, 2019

+1

@cmsbuild cmsbuild merged commit 5ba8d6f into cms-sw:master Jun 5, 2019
@perrotta perrotta deleted the adjustClangFormatUpdates branch June 5, 2019 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants