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
[NanoAOD] Don't handle column types redundantly anymore #30436
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
2a20e22
don't handle column types redundantly anymore
guitargeek 1e1f1ad
make ColumnType and enum class for type safety
guitargeek 0399a0f
fix clang warning in SimpleFlatTableProducer.h
guitargeek ef17616
reorganize more redundant checks in FlatTable
guitargeek d147331
throw exceptions in NanoAOD if column type not supported
guitargeek ea89dc3
Avoid use of const_cast with helper function
guitargeek 43dc9e4
introduce edm::Span
guitargeek 00b26e7
change unneeded universal reference to const&
guitargeek File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
#ifndef FWCore_Utilities_Span_h | ||
#define FWCore_Utilities_Span_h | ||
|
||
#include <cstddef> | ||
|
||
namespace edm { | ||
/* | ||
*An edm::Span wraps begin() and end() iterators to a contiguous sequence | ||
of objects with the first element of the sequence at position zero, | ||
In other words the iterators should refer to random-access containers. | ||
|
||
To be replaced with std::Span in C++20. | ||
*/ | ||
|
||
template <class T> | ||
class Span { | ||
public: | ||
Span(T begin, T end) : begin_(begin), end_(end) {} | ||
|
||
T begin() const { return begin_; } | ||
T end() const { return end_; } | ||
|
||
bool empty() const { return begin_ == end_; } | ||
auto size() const { return end_ - begin_; } | ||
|
||
auto const& operator[](std::size_t idx) const { return *(begin_ + idx); } | ||
|
||
auto const& front() const { return *begin_; } | ||
auto const& back() const { return *(end_ - 1); } | ||
|
||
private: | ||
const T begin_; | ||
const T end_; | ||
}; | ||
}; // namespace edm | ||
|
||
#endif |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why rvalue reference instead of
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I once read this blog post about move semantics:
https://eli.thegreenplace.net/2014/perfect-forwarding-and-universal-references-in-c/#id6
Specifically the part "Don't let
T&&
fool you here ...". So it's not a rvalue reference, but makes the function automatically deduce if taking the argument by value or by reference is appropriate. Therefore it appeared to me that it might be a good habit to use&&
by default for template deduced arguments, especially in this class wherebulk
is later used with an rvalue.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
T&&
is sometimes referred to as the universal reference.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this isocpp blogpost is a more official reference for these universal references:
https://isocpp.org/blog/2012/11/universal-references-in-c11-scott-meyers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, forgot that detail, never mind then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this particular case I do not see a need for the universal reference as the type is directly interacted with instead of passed to another function via the use of
std::forward
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's also true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it's such a minor change that I think I can also do it later in one go when I rebase this PR for the nanoAOD repository, or if you have more major comments, if this is okay.
Speaking about perfect forwarding, I recently noticed in the PR where I used FWCore/SOA that the SOA Table might make several unnecessary copies because even though
std::forward
is used, this is not matched by the corresponding universal reference for the argument, for example here (and also in other functions for the implementation details):https://github.com/cms-sw/cmssw/blob/master/FWCore/SOA/interface/Table.h#L145
I didn't open an issue about this because I didn't think it was that important, but I it's just something to keep in mind for the next time anyone will work with FWCore/SOA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfectly fine for me, it is minor indeed.
The best way to remind us (
core
) is to open an issue :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok thanks!