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.

Sign up for GitHub

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

Jump to bottom

turn getenv into std::getenv for all cmssw residual occurencies #28124

Merged
merged 2 commits into from Oct 11, 2019
Merged

turn getenv into std::getenv for all cmssw residual occurencies #28124

merged 2 commits into from Oct 11, 2019

Conversation

fabiocos
Copy link
Contributor

@fabiocos fabiocos commented Oct 7, 2019

PR description:

This PR addresses #28073 , following #28117 that fixes it for the FWCore area.

PR validation:

The code compiles.

@cmsbuild cmsbuild added this to the CMSSW_11_0_X milestone Oct 7, 2019
@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 7, 2019

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 7, 2019

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28124/12165

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 8, 2019

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 8, 2019

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28124/12166

@cmsbuild
Copy link
Contributor

cmsbuild commented Oct 8, 2019

A new Pull Request was created by @fabiocos (Fabio Cossutti) for master.

It involves the following packages:

Alignment/OfflineValidation
CalibCalorimetry/EcalCorrelatedNoiseAnalysisAlgos
CalibCalorimetry/EcalLaserAnalyzer
CalibFormats/SiPixelObjects
CondCore/CondDB
CondFormats/JetMETObjects
DQM/SiStripCommissioningClients
DQM/SiStripCommissioningSources
DQMServices/StreamerIO
DetectorDescription/RegressionTest
EventFilter/DTRawToDigi
EventFilter/Utilities
Fireworks/FWInterface
GeneratorInterface/EvtGenInterface
GeneratorInterface/Pythia8Interface
GeneratorInterface/RivetInterface
GeneratorInterface/SherpaInterface
IOMC/ParticleGuns
L1Trigger/CSCCommonTrigger
L1Trigger/L1TMuonBarrel
OnlineDB/CSCCondDB
OnlineDB/SiStripConfigDb
PhysicsTools/TensorFlow
PhysicsTools/Utilities
RecoLuminosity/LumiProducer
RecoMET/METAlgorithms
TopQuarkAnalysis/TopHitFit
Utilities/DavixAdaptor
Utilities/XrdAdaptor

@SiewYan, @andrius-k, @emeschi, @schneiml, @ianna, @rekovic, @fioriNTU, @tlampen, @alberto-sanchez, @pohsun, @santocch, @perrotta, @civanch, @cmsbuild, @agrohsje, @smuzaffar, @Dr15Jones, @cvuosalo, @efeyazgan, @mdhildreth, @jfernan2, @tocheng, @slava77, @ggovi, @qliphy, @benkrikler, @mkirsano, @kmaeshima, @christopheralanwest, @alja, @franzoni, @mommsen can you please review it and eventually sign? Thanks.
@erikbutz, @rappoccio, @gouskos, @argiro, @Martin-Grunewald, @thuer, @thomreis, @tlampen, @alberto-sanchez, @ahinzmann, @threus, @seemasharmafnal, @mmarionncern, @kreczko, @makortel, @jdolen, @agrohsje, @dildick, @barvic, @tocheng, @jdamgov, @nhanvtran, @gkasieczka, @schoef, @mschrode, @mmusich, @clelange, @mkirsano, @adewit, @riga, @wddgit, @alja, @mariadalfonso 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

@perrotta
Copy link
Contributor

perrotta commented Oct 8, 2019

+1

  • Technical
  • Jenkins tests pass
  • (reco part of this PR not even heavily affected by the code-format changes, which are responsible of the largest number of line changes here)

@alja
Copy link
Contributor

alja commented Oct 8, 2019

+1

ianna
ianna reviewed Oct 8, 2019
View reviewed changes
DetectorDescription/RegressionTest/src/DDExpandedViewDump.cc
@@ -22,7 +22,7 @@ void DDExpandedViewDump(ostream& os, DDExpandedView& ex, size_t skip, size_t sto
bool go(true);
int count(0);
bool dotrans(true);
if (getenv("DDNOTRANS"))
if (std::getenv("DDNOTRANS"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Wasn't it an std::getenv used here? Given that line #1 includes <cstdlib> and line #17 defines that the std namespace is used - otherwise, the compiler would have complained.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ianna in this specific case it looks redundant, although not incorrect I would say

@Dr15Jones Dr15Jones mentioned this pull request Oct 8, 2019
Closed
@fabiocos
Copy link
Contributor Author

fabiocos commented Oct 9, 2019

@christopheralanwest @pohsun @qliphy @agrohsje @ggovi @santocch @rekovic please have a look, I would like to move forward with this PR asap

@christopheralanwest
Copy link
Contributor

+1

@qliphy
Copy link
Contributor

qliphy commented Oct 10, 2019

+1

@ianna
Copy link
Contributor

ianna commented Oct 10, 2019

+1

@cmsbuild cmsbuild mentioned this pull request Oct 10, 2019
Merged
@fabiocos
Copy link
Contributor Author

+1

@fabiocos
Copy link
Contributor Author

merge

@santocch @ggovi @rekovic the changes are purely technical and should imply no regression, please check

@cmsbuild cmsbuild merged commit 011cdfd into cms-sw:master Oct 11, 2019
This was referenced Oct 11, 2019
Merged
Merged
@santocch
Copy link

+1

@fabiocos fabiocos deleted the fc-getenv branch December 18, 2019 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ianna ianna ianna left review comments

Assignees
No one assigned
Labels
alca-approved analysis-approved code-checks-approved comparison-available core-approved daq-approved db-pending dqm-approved generators-approved geometry-approved l1-pending orp-approved pending-signatures reconstruction-approved tests-approved visualization-approved
Projects
None yet
Milestone
CMSSW_11_0_X
Development

Successfully merging this pull request may close these issues.

None yet