Skip to content

Removed -Wconversion warnings in part of framework #47339

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

Closed
wants to merge 1 commit into from

Conversation

Dr15Jones
Copy link
Contributor

PR description:

Primarily did lowest level packages such as most DataFormats maintained by Core and FWCore/Utilities.

PR validation:

Code compiles. Doing USER_CXXFLAGS=-Wconversion scram b showed the changes avoid the warnings.

@cmsbuild
Copy link
Contributor

cmsbuild commented Feb 12, 2025

cms-bot internal usage

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47339/43672

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

Primarily did lowest level packages such as most DataFormats
maintained by Core and FWCore/Utilities.
@Dr15Jones
Copy link
Contributor Author

please test

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47339/43673

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • DataFormats/Common (core)
  • DataFormats/Provenance (core)
  • FWCore/Common (core)
  • FWCore/Concurrency (core)
  • FWCore/FWLite (core)
  • FWCore/Framework (core)
  • FWCore/Integration (core)
  • FWCore/MessageLogger (core)
  • FWCore/MessageService (core)
  • FWCore/ParameterSet (core)
  • FWCore/PrescaleService (core)
  • FWCore/PythonParameterSet (core)
  • FWCore/Reflection (core)
  • FWCore/SOA (core)
  • FWCore/Services (core)
  • FWCore/SharedMemory (core)
  • FWCore/Sources (core)
  • FWCore/TestProcessor (core)
  • FWCore/Utilities (core)
  • IOMC/RandomEngine (core)
  • IOPool/Input (core)
  • IOPool/Streamer (core)
  • IOPool/TFileAdaptor (core)
  • SimDataFormats/RandomEngine (simulation)
  • Utilities/General (core)

@Dr15Jones, @civanch, @kpedro88, @makortel, @mdhildreth, @smuzaffar can you please review it and eventually sign? Thanks.
@apsallid, @bsunanda, @denizsun, @fabiocos, @felicepantaleo, @fwyzard, @makortel, @martinamalberti, @missirol, @mmusich, @rovere, @salimcerci, @wddgit, @youyingli this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

@@ -126,17 +126,18 @@ namespace edm {

const_IterPair pair(size_t i) const { return const_IterPair(m_ids.begin() + i, m_data.begin() + i * m_stride); }

DataFrame operator[](size_t i) { return DataFrame(*this, i); }
DataFrame operator[](size_t i) { return DataFrame(*this, static_cast<DataFrame::size_type>(i)); }
Copy link
Contributor

Choose a reason for hiding this comment

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

In these cases, wouldn't it be better to change the function parameter type to match what is expected ?
That is,

Suggested change
DataFrame operator[](size_t i) { return DataFrame(*this, static_cast<DataFrame::size_type>(i)); }
DataFrame operator[](DataFrame::size_type i) { return DataFrame(*this, i); }

?


/// Get number of paths stored
unsigned int size() const { return paths_.size(); }
auto size() const { return paths_.size(); }
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
auto size() const { return paths_.size(); }
size_type size() const { return paths_.size(); }

?

Comment on lines +43 to +44
const auto n(size());
for (decltype(size()) i = 0; i != n; ++i)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const auto n(size());
for (decltype(size()) i = 0; i != n; ++i)
const size_type n = size();
for (size_type i = 0; i != n; ++i)

?

m_item.offset = offset;

m_v.m_dataSize = m_v.m_data.size();
m_v.m_dataSize = static_cast<unsigned int>(m_v.m_data.size());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
m_v.m_dataSize = static_cast<unsigned int>(m_v.m_data.size());
m_v.m_dataSize = static_cast<size_type>(m_v.m_data.size());

?

@@ -451,7 +451,7 @@ namespace edmNew {
void resize(size_t isize, size_t dsize) {
m_ids.resize(isize);
m_data.resize(dsize);
m_dataSize = m_data.size();
m_dataSize = static_cast<unsigned int>(m_data.size());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
m_dataSize = static_cast<unsigned int>(m_data.size());
m_dataSize = static_cast<size_type>(m_data.size());

?

@@ -463,14 +463,14 @@ namespace edmNew {
Item& item = addItem(iid, isize);
m_data.resize(m_data.size() + isize);
std::copy(idata, idata + isize, m_data.begin() + item.offset);
m_dataSize = m_data.size();
m_dataSize = static_cast<unsigned int>(m_data.size());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
m_dataSize = static_cast<unsigned int>(m_data.size());
m_dataSize = static_cast<size_type>(m_data.size());

?

return DetSet(*this, item, false);
}
//make space for it
DetSet insert(id_type iid, size_type isize) {
Item& item = addItem(iid, isize);
m_data.resize(m_data.size() + isize);
m_dataSize = m_data.size();
m_dataSize = static_cast<unsigned int>(m_data.size());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
m_dataSize = static_cast<unsigned int>(m_data.size());
m_dataSize = static_cast<size_type>(m_data.size());

?

@@ -88,10 +88,10 @@ namespace edm {
m_keys.push_back(k);
m_data.resize(m_offsets.back() + v.size());
std::copy(v.begin(), v.end(), m_data.begin() + m_offsets.back());
m_offsets.push_back(m_data.size());
m_offsets.push_back(static_cast<unsigned int>(m_data.size()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
m_offsets.push_back(static_cast<unsigned int>(m_data.size()));
m_offsets.push_back(static_cast<size_type>(m_data.size()));

?

@@ -103,7 +103,7 @@ namespace edm {
static constexpr unsigned int s_uninitializedValue = 0xFFFFFFFF;

constexpr explicit EDPutTokenT(unsigned int iValue) noexcept : m_value(iValue) {}
constexpr explicit EDPutTokenT(unsigned long int iValue) noexcept : m_value(iValue) {}
constexpr explicit EDPutTokenT(unsigned long int iValue) noexcept : m_value(static_cast<unsigned int>(iValue)) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need the unsigned long int overload?

@@ -32,7 +32,7 @@ namespace edm {

template <>
constexpr bool isFinite(long double x) {
double xx = x;
double xx = static_cast<double>(x);
return isFinite(xx);
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this long double overload really works properly (in presence of fast math), but ok, whatever the behavior is, it is there already.

@@ -113,10 +113,12 @@ CPUTimer::Times CPUTimer::calculateDeltaTime() const {
struct timespec tp;

clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &tp);
returnValue.cpu_ = tp.tv_sec - startCPUTime_.tv_sec + nanosecToSec * (tp.tv_nsec - startCPUTime_.tv_nsec);
returnValue.cpu_ =
double(tp.tv_sec - startCPUTime_.tv_sec) + nanosecToSec * double(tp.tv_nsec - startCPUTime_.tv_nsec);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could static_cast be used here?

returnValue = theUsage.ru_stime.tv_sec + theUsage.ru_utime.tv_sec - startCPUTime_.tv_sec +
microsecToSec * (theUsage.ru_stime.tv_usec + theUsage.ru_utime.tv_usec - startCPUTime_.tv_usec);
returnValue = double(theUsage.ru_stime.tv_sec + theUsage.ru_utime.tv_sec - startCPUTime_.tv_sec) +
microsecToSec * double(theUsage.ru_stime.tv_usec + theUsage.ru_utime.tv_usec - startCPUTime_.tv_usec);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could astatic_cast be used here?

@@ -89,7 +89,7 @@ void testCPUTimer::testTiming()
getrusage(RUSAGE_SELF, &theUsage2);
nowTime.tv_sec = theUsage2.ru_utime.tv_sec;
nowTime.tv_usec = theUsage2.ru_utime.tv_usec;
} while (nowTime.tv_sec - startTime.tv_sec + 1E-6 * (nowTime.tv_usec - startTime.tv_usec) < 1);
} while (double(nowTime.tv_sec - startTime.tv_sec) + 1E-6 * double(nowTime.tv_usec - startTime.tv_usec) < 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

static_cast?

@cmsbuild
Copy link
Contributor

-1

Failed Tests: Build
Size: This PR adds an extra 796KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-49d04e/44365/summary.html
COMMIT: bcf2c09
CMSSW: CMSSW_15_1_X_2025-02-12-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/47339/44365/install.sh to create a dev area with all the needed externals and cmssw changes.

Build

I found compilation error when building:

>> Compiling edm plugin src/PerfTools/AllocMonitor/plugins/moduleAlloc_setupFile.cc
/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02876/el8_amd64_gcc12/external/gcc/12.3.1-40d504be6370b5a30e3947a6e575ca28/bin/c++ -c -DCMS_MICRO_ARCH='x86-64-v3' -DGNU_GCC -D_GNU_SOURCE -DTBB_USE_GLIBCXX_VERSION=120301 -DTBB_SUPPRESS_DEPRECATED_MESSAGES -DTBB_PREVIEW_RESUMABLE_TASKS=1 -DTBB_PREVIEW_TASK_GROUP_EXTENSIONS=1 -DBOOST_SPIRIT_THREADSAFE -DPHOENIX_THREADSAFE -DBOOST_MATH_DISABLE_STD_FPCLASSIFY -DBOOST_UUID_RANDOM_PROVIDER_FORCE_POSIX -DCMSSW_GIT_HASH='CMSSW_15_1_X_2025-02-12-1100' -DPROJECT_NAME='CMSSW' -DPROJECT_VERSION='CMSSW_15_1_X_2025-02-12-1100' -Isrc -Ipoison -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02876/el8_amd64_gcc12/cms/cmssw-patch/CMSSW_15_1_X_2025-02-12-1100/src -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02876/el8_amd64_gcc12/external/pcre/8.43-2d141998cfe5424b8f7aff48035cc2da/include -isystem/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02876/el8_amd64_gcc12/external/boost/1.80.0-96a02191111b66819e07de179cb25a0e/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02876/el8_amd64_gcc12/external/bz2lib/1.0.6-d065ccd79984efc6d4660f410e4c81de/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02876/el8_amd64_gcc12/external/libuuid/2.34-27ce4c3579b5b1de2808ea9c4cd8ed29/include -isystem/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02876/el8_amd64_gcc12/lcg/root/6.32.09-0a945d6e24dcaabe218b38fa8292785d/include -isystem/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02876/el8_amd64_gcc12/external/tbb/v2021.9.0-9b0f135342cfe979b6500c59f501774e/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02876/el8_amd64_gcc12/external/xz/5.2.5-6f3f49b07db84e10c9be594a1176c114/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02876/el8_amd64_gcc12/external/zlib/1.2.13-d217cdbdd8d586e845e05946de2796be/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02876/el8_amd64_gcc12/external/fmt/10.2.1-e35fd1db5eb3abc8ac0452e8ee427196/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02876/el8_amd64_gcc12/external/md5/1.0.0-5b594b264e04ae51e893b1d69a797ec6/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02876/el8_amd64_gcc12/external/tinyxml2/6.2.0-f99ae2781d074227d47e8a3e7c8ec87e/include -O3 -pthread -pipe -Werror=main -Werror=pointer-arith -Werror=overlength-strings -Wno-vla -Werror=overflow -std=c++20 -ftree-vectorize -Werror=array-bounds -Werror=format-contains-nul -Werror=type-limits -fvisibility-inlines-hidden -fno-math-errno --param vect-max-version-for-alias-checks=50 -Xassembler --compress-debug-sections -Wno-error=array-bounds -Warray-bounds -fuse-ld=bfd -march=x86-64-v3 -felide-constructors -fmessage-length=0 -Wall -Wno-non-template-friend -Wno-long-long -Wreturn-type -Wextra -Wpessimizing-move -Wclass-memaccess -Wno-cast-function-type -Wno-unused-but-set-parameter -Wno-ignored-qualifiers -Wno-unused-parameter -Wunused -Wparentheses -Werror=return-type -Werror=missing-braces -Werror=unused-value -Werror=unused-label -Werror=address -Werror=format -Werror=sign-compare -Werror=write-strings -Werror=delete-non-virtual-dtor -Werror=strict-aliasing -Werror=narrowing -Werror=unused-but-set-variable -Werror=reorder -Werror=unused-variable -Werror=conversion-null -Werror=return-local-addr -Wnon-virtual-dtor -Werror=switch -fdiagnostics-show-option -Wno-unused-local-typedefs -Wno-attributes -Wno-psabi -Wno-error=unused-variable -DBOOST_DISABLE_ASSERTS -flto=auto -fipa-icf -flto-odr-type-merging -fno-fat-lto-objects -Wodr -fPIC -MMD -MF tmp/el8_amd64_gcc12/src/PerfTools/AllocMonitor/plugins/PerfToolsAllocMonitorAuto/moduleAlloc_setupFile.cc.d src/PerfTools/AllocMonitor/plugins/moduleAlloc_setupFile.cc -o tmp/el8_amd64_gcc12/src/PerfTools/AllocMonitor/plugins/PerfToolsAllocMonitorAuto/moduleAlloc_setupFile.cc.o
>> Compiling edm plugin src/PerfTools/AllocMonitor/plugins/monitor_file_utilities.cc
/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02876/el8_amd64_gcc12/external/gcc/12.3.1-40d504be6370b5a30e3947a6e575ca28/bin/c++ -c -DCMS_MICRO_ARCH='x86-64-v3' -DGNU_GCC -D_GNU_SOURCE -DTBB_USE_GLIBCXX_VERSION=120301 -DTBB_SUPPRESS_DEPRECATED_MESSAGES -DTBB_PREVIEW_RESUMABLE_TASKS=1 -DTBB_PREVIEW_TASK_GROUP_EXTENSIONS=1 -DBOOST_SPIRIT_THREADSAFE -DPHOENIX_THREADSAFE -DBOOST_MATH_DISABLE_STD_FPCLASSIFY -DBOOST_UUID_RANDOM_PROVIDER_FORCE_POSIX -DCMSSW_GIT_HASH='CMSSW_15_1_X_2025-02-12-1100' -DPROJECT_NAME='CMSSW' -DPROJECT_VERSION='CMSSW_15_1_X_2025-02-12-1100' -Isrc -Ipoison -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02876/el8_amd64_gcc12/cms/cmssw-patch/CMSSW_15_1_X_2025-02-12-1100/src -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02876/el8_amd64_gcc12/external/pcre/8.43-2d141998cfe5424b8f7aff48035cc2da/include -isystem/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02876/el8_amd64_gcc12/external/boost/1.80.0-96a02191111b66819e07de179cb25a0e/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02876/el8_amd64_gcc12/external/bz2lib/1.0.6-d065ccd79984efc6d4660f410e4c81de/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02876/el8_amd64_gcc12/external/libuuid/2.34-27ce4c3579b5b1de2808ea9c4cd8ed29/include -isystem/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02876/el8_amd64_gcc12/lcg/root/6.32.09-0a945d6e24dcaabe218b38fa8292785d/include -isystem/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02876/el8_amd64_gcc12/external/tbb/v2021.9.0-9b0f135342cfe979b6500c59f501774e/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02876/el8_amd64_gcc12/external/xz/5.2.5-6f3f49b07db84e10c9be594a1176c114/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02876/el8_amd64_gcc12/external/zlib/1.2.13-d217cdbdd8d586e845e05946de2796be/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02876/el8_amd64_gcc12/external/fmt/10.2.1-e35fd1db5eb3abc8ac0452e8ee427196/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02876/el8_amd64_gcc12/external/md5/1.0.0-5b594b264e04ae51e893b1d69a797ec6/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02876/el8_amd64_gcc12/external/tinyxml2/6.2.0-f99ae2781d074227d47e8a3e7c8ec87e/include -O3 -pthread -pipe -Werror=main -Werror=pointer-arith -Werror=overlength-strings -Wno-vla -Werror=overflow -std=c++20 -ftree-vectorize -Werror=array-bounds -Werror=format-contains-nul -Werror=type-limits -fvisibility-inlines-hidden -fno-math-errno --param vect-max-version-for-alias-checks=50 -Xassembler --compress-debug-sections -Wno-error=array-bounds -Warray-bounds -fuse-ld=bfd -march=x86-64-v3 -felide-constructors -fmessage-length=0 -Wall -Wno-non-template-friend -Wno-long-long -Wreturn-type -Wextra -Wpessimizing-move -Wclass-memaccess -Wno-cast-function-type -Wno-unused-but-set-parameter -Wno-ignored-qualifiers -Wno-unused-parameter -Wunused -Wparentheses -Werror=return-type -Werror=missing-braces -Werror=unused-value -Werror=unused-label -Werror=address -Werror=format -Werror=sign-compare -Werror=write-strings -Werror=delete-non-virtual-dtor -Werror=strict-aliasing -Werror=narrowing -Werror=unused-but-set-variable -Werror=reorder -Werror=unused-variable -Werror=conversion-null -Werror=return-local-addr -Wnon-virtual-dtor -Werror=switch -fdiagnostics-show-option -Wno-unused-local-typedefs -Wno-attributes -Wno-psabi -Wno-error=unused-variable -DBOOST_DISABLE_ASSERTS -flto=auto -fipa-icf -flto-odr-type-merging -fno-fat-lto-objects -Wodr -fPIC -MMD -MF tmp/el8_amd64_gcc12/src/PerfTools/AllocMonitor/plugins/PerfToolsAllocMonitorAuto/monitor_file_utilities.cc.d src/PerfTools/AllocMonitor/plugins/monitor_file_utilities.cc -o tmp/el8_amd64_gcc12/src/PerfTools/AllocMonitor/plugins/PerfToolsAllocMonitorAuto/monitor_file_utilities.cc.o
src/PerfTools/AllocMonitor/plugins/monitor_file_utilities.cc: In function 'void edm::moduleAlloc::monitor_file_utilities::moduleIdToLabel(std::ostream&, const std::vector >&, char, const std::string&, const std::string&)':
src/PerfTools/AllocMonitor/plugins/monitor_file_utilities.cc:17:35: error: narrowing conversion of '(std::size_t)width' from 'std::size_t' {aka 'long unsigned int'} to 'int' [-Werror=narrowing]
   17 |     OStreamColumn col0{iIDHeader, width};
      |                                   ^~~~~
cc1plus: some warnings being treated as errors
gmake: *** [tmp/el8_amd64_gcc12/src/PerfTools/AllocMonitor/plugins/PerfToolsAllocMonitorAuto/monitor_file_utilities.cc.o] Error 1
>> Building edm plugin tmp/el8_amd64_gcc12/src/PerfTools/AllocMonitor/plugins/PerfToolsAllocMonitorAuto/libPerfToolsAllocMonitorAuto.so


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.

4 participants