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

Update propagate_const and add propagate_const_array #32184

Merged
merged 2 commits into from Nov 25, 2020
Merged

Update propagate_const and add propagate_const_array #32184

merged 2 commits into from Nov 25, 2020

Conversation

fwyzard
Copy link
Contributor

@fwyzard fwyzard commented Nov 19, 2020 •
edited

PR description:

Update edm::propagate_const<...> and its unit test:

  • make most methods of edm::propagate_const constexpr;
  • add a unit test for edm::propagate_const<incomplete_type *>.

Add edm::propagate_const_array<...> to support T[], unique_ptr<T[]>, etc.

Implement edm::propagate_const_array template with similar functionality to edm::propagate_const, but with a subscript operator and support for types like T[], std::unique_ptr<T[]> and similar array-like types.

Add support for get_underlying_safe<propagate_const_array<T>>.

Add unit tests for edm::propagate_const_array:

  • propagate_const_array<T[]>
  • propagate_const_array<unique_ptr<T[]>>
  • propagate_const_array<shared_ptr<T[]>>
  • propagate_const_array<incomplete_type[]>

PR validation

Unit test testFWCoreUtilities builds and runs.

@cmsbuild cmsbuild added this to the CMSSW_11_2_X milestone Nov 19, 2020
@fwyzard
Copy link
Contributor Author

fwyzard commented Nov 19, 2020

These proposal stems from the discussion at #32039 and cms-patatrack#574 .

It turns out that edm::propagate_const (as well as std::experimental::propagate_const) do not support types with a subscript operator [] but without an indirection operator * - for example, std::unique_ptr<T[]>.

The latter can be useful to guarantee that the conditions data are not modified by accident, by propagating the constness of their container to the actual data.

@fwyzard
Copy link
Contributor Author

fwyzard commented Nov 19, 2020

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32184/19914

  • This PR adds an extra 20KB to repository

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

@fwyzard fwyzard mentioned this pull request Nov 19, 2020
Merged
@fwyzard
Copy link
Contributor Author

fwyzard commented Nov 19, 2020

@cmsbuild, please test

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32184/19915

  • This PR adds an extra 20KB to repository

@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 19, 2020 •
edited

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @fwyzard (Andrea Bocci) for master.

It involves the following packages:

FWCore/Utilities

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

cms-bot commands are listed here

@makortel
Copy link
Contributor

@Dr15Jones Could you review this one?

@Dr15Jones
Copy link
Contributor

Although I think this change is pretty neat and definitely does something very useful, my concern is edm::propagate_const was meant to be replaced with std::propagate_const once (if?) it enters the standard from the std::experimental namespace. As such the std::propagate_const does not support arrays, see:
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/n4853.html#propagate_const

Though introduced in C++17, the std::propagate_const was not included in C++20 (although it is in std::experimental for C++20). I'm not sure what this means for the future of that implementation.

As a possible way to still allow edm::propagate_const to be replaced with std::propagate_const (if that future happens) how about creating a separate edm::propagate_const_array class?

@fwyzard
Copy link
Contributor Author

fwyzard commented Nov 19, 2020

Them... we make a PR for the standard ?

@cmsbuild
Copy link
Contributor

+1
Tested at: 9497771
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-97d2ff/10867/summary.html
CMSSW: CMSSW_11_2_X_2020-11-19-1100
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link
Contributor

Comparison job queued.

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (but tests are reportedly failing). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

fwyzard added a commit to cms-patatrack/cmssw that referenced this pull request Nov 24, 2020
@fwyzard
Backport: update propagate_const and add propagate_const_array (cms-s…
36b2188 
…w#32184)

Make most methods of edm::propagate_const<T> constexpr.
Add a unit test for edm::propagate_const<incomplete_type *>.

Implement edm::propagate_const_array template with similar functionality
to edm::propagate_const, but with a subscript operator and support for
types like T[], std::unique_ptr<T[]>> and similar array-like types.

Add support for get_underlying_safe<propagate_const_array<T>>.

Add unit tests for edm::propagate_const_array:
  - propagate_const_array<T[]>
  - propagate_const_array<unique_ptr<T[]>>
  - propagate_const_array<shared_ptr<T[]>>
  - propagate_const_array<incomplete_type[]>
@fwyzard fwyzard mentioned this pull request Nov 24, 2020
Merged
@cmsbuild
Copy link
Contributor

cmsbuild commented Nov 24, 2020 •
edited

The tests are being triggered in jenkins.
Tested with other pull request(s) #32261

@fwyzard
Copy link
Contributor Author

fwyzard commented Nov 24, 2020

@silviodonato @qliphy can we have this in pre10 ?
It would simplify the various Patatrack PRs currently under review.

@silviodonato
Copy link
Contributor

urgent

@fwyzard fwyzard mentioned this pull request Nov 24, 2020
Merged
@mrodozov
Copy link
Contributor

---> test TestRunnerFWCoreTFWLiteSelector had ERRORS

Mhm, is this actually related to this PR ?

Unlikely, we've seen that unit test to fail in ROOT6 master IBs (#31233, #32162) and apparently sporadically for years (#31233 (comment)).

well in default IBs it will be a ... unpleasant to brake PRs

@fwyzard
Copy link
Contributor Author

fwyzard commented Nov 24, 2020

well in default IBs it will be a ... unpleasant to brake PRs

@mrodozov uhm... what ?

@mrodozov
Copy link
Contributor

well in default IBs it will be a ... unpleasant to brake PRs

@mrodozov uhm... what ?

if this failing test start breaking more often in the default IBs (which it didn't do so far, it was failing in ROOT6 IB only)
it will start bothering people. with their PR working but the test failing. like in this pr

@cmsbuild
Copy link
Contributor

+1
Tested at: d5169fc
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-701374/11000/summary.html
CMSSW: CMSSW_11_2_X_2020-11-23-2300
SCRAM_ARCH: slc7_amd64_gcc820

@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-701374/11000/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 37
  • DQMHistoTests: Total histograms compared: 2961011
  • DQMHistoTests: Total failures: 7
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2960982
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
  • Checked 158 log files, 34 edm output root files, 37 DQM output files

@qliphy
Copy link
Contributor

qliphy commented Nov 25, 2020

+1

@cmsbuild cmsbuild merged commit f261e3a into cms-sw:master Nov 25, 2020
@cmsbuild cmsbuild mentioned this pull request Nov 25, 2020
Merged
@fwyzard fwyzard deleted the extend_propagate_const branch December 27, 2020 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
code-checks-approved comparison-available core-approved fully-signed orp-approved requires-external tests-approved urgent
Projects
None yet
Milestone
CMSSW_11_2_X
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@fwyzard @cmsbuild @makortel @Dr15Jones @silviodonato @mrodozov @qliphy