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

[Clang tidy] Apply checks for core #30712

Closed
wants to merge 1 commit into from

Conversation

cmsbuild
Copy link
Contributor

Apply new clang checks. See #30508 for more info.

@cmsbuild
Copy link
Contributor Author

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor Author

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30712/17027

  • This PR adds an extra 12KB to repository

@cmsbuild
Copy link
Contributor Author

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

It involves the following packages:

DataFormats/Common
DataFormats/FWLite
DataFormats/Provenance
DataFormats/TestObjects
FWCore/Catalog
FWCore/Concurrency
FWCore/FWLite
FWCore/Framework
FWCore/Integration
FWCore/MessageService
FWCore/ParameterSet
FWCore/PluginManager
FWCore/PyDevParameterSet
FWCore/PythonParameterSet
FWCore/SOA
FWCore/ServiceRegistry
FWCore/SharedMemory
FWCore/TestProcessor
FWCore/Utilities
IOMC/RandomEngine
IOPool/TFileAdaptor
PerfTools/Callgrind
PerfTools/EdmEvent
Utilities/DavixAdaptor
Utilities/StorageFactory

@makortel, @smuzaffar, @cmsbuild, @Dr15Jones can you please review it and eventually sign? Thanks.
@makortel, @felicepantaleo, @rovere, @wddgit, @fwyzard, @fabiocos this is something you requested to watch as well.
@silviodonato, @dpiparo you are the release manager for this.

cms-bot commands are listed here

@makortel
Copy link
Contributor

@cmsbuild, please test

@cmsbuild
Copy link
Contributor Author

cmsbuild commented Jul 15, 2020

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor Author

-1

Tested at: 30f9777

CMSSW: CMSSW_11_2_X_2020-07-14-2300
SCRAM_ARCH: slc7_amd64_gcc820
You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-b69d89/7979/summary.html

I found follow errors while testing this PR

Failed tests: Build ClangBuild

  • Build:

I found compilation warning when building: See details on the summary page.

  • Clang:

I found compilation warning while trying to compile with clang. Command used:

USER_CUDA_FLAGS='--expt-relaxed-constexpr' USER_CXXFLAGS='-Wno-register -fsyntax-only' scram build -k -j 32 COMPILER='llvm compile'

See details on the summary page.

@cmsbuild
Copy link
Contributor Author

Comparison not run due to Build errors (RelVals and Igprof tests were also skipped)

@@ -640,11 +640,11 @@ bool testMultiAssociation::tryBadFill(int i) {
} break;
case 11: { // Can copy a LazyFiller, but crash if I fill twice
MultiRef::LazyFiller filler = m.lazyFiller(handleK1, true);
MultiRef::LazyFiller filler2 = filler;
const MultiRef::LazyFiller& filler2 = filler;
Copy link
Contributor

Choose a reason for hiding this comment

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

Apparently we need to add

Suggested change
const MultiRef::LazyFiller& filler2 = filler;
const MultiRef::LazyFiller& filler2 = filler; //NOLINT

here.

@smuzaffar @mrodozov Should that be done in a separate PR and then we refresh this one?

Copy link
Contributor

@makortel makortel left a comment

Choose a reason for hiding this comment

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

All the warnings appear to be from cases where a copy is intentional and should not be changed to const reference.

} break;
case 12: { // Can copy a FastFiller
MultiRef::FastFiller filler = m.fastFiller(handleK1);
MultiRef::FastFiller filler2 = filler;
const MultiRef::FastFiller& filler2 = filler;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

CPPUNIT_ASSERT(4 == test.getParameter<std::string>("sb3").size());
std::vector<std::string> vs = test.getParameter<std::vector<std::string> >("vs");
int vssize = vs.size();
//FIXME doesn't do spaces right
edm::Entry e(test.retrieve("vs"));
const edm::Entry& e(test.retrieve("vs"));
Copy link
Contributor

Choose a reason for hiding this comment

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

and here

CPPUNIT_ASSERT(4 == test.getParameter<std::string>("sb3").size());
std::vector<std::string> vs = test.getParameter<std::vector<std::string> >("vs");
int vssize = vs.size();
//FIXME doesn't do spaces right
edm::Entry e(test.retrieve("vs"));
const edm::Entry& e(test.retrieve("vs"));
Copy link
Contributor

Choose a reason for hiding this comment

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

and here

@mrodozov
Copy link
Contributor

yes unfortunately the checks were not applied properly but I found that after making the PR :/ I'm fixing it and force pushing the updates

@cmsbuild cmsbuild closed this Jul 17, 2020
@cmsbuild cmsbuild deleted the apply-new-ct-checks-for-core branch July 17, 2020 16:49
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

3 participants