Code Review with Accompanying Recommendations

[jcfreeman2]

Overview

This is a code review as part of the larger project of reviewing the various data formats packages, as described in https://github.com/DUNE-DAQ/daq-deliverables/issues/215. For details / motivations behind my suggestions, please click the link. In this Issue, I'll start by providing some general comments about the repository, then I'll go file-by-file.

General Comments

Documentation

Straightforward improvements here would be updating the links in the main README.md to the most up-to-date versions of their targets (e.g., clicking on "FragmentHeader description" from the home page points you to the v4 version of the fragment header, whereas we're up to v6. Also, not all the diagrams are completely up to date - e.g., here, it's v5 of FragmentHeader which is displayed.

Less importantly, it might be nice to explain what "component requests" are in a short sentence when discussing the TriggerRecordHeader.

As far as in-source commenting, I think it can be reduced in some places. E.g., comments like this

 /**                                                                                                                                
   * @brief Fragment destructor                                                                                                      
   */
  inline ~Fragment();

are probably overkill, as is

 /**                                                                                                                                
   * @brief Set the run_number for the Fragment                                                                                      
   * @param run_number Value of run_number to set                                                                                    
   */
  void set_run_number(run_number_t run_number) { header_()->run_number = run_number; }

Additionally, I believe that for one-line comments we want in Doxygen, the /// @ brief syntax can be a bit more terse than /**.

Relatedly, in CMakeLists.txt, there's some superfluous commenting - e.g.

##############################################################################                              
# Plugins

...when the package doesn't have any plugins.

Static assertions using offsetof to check byte location

Obviously, the effects of unexpected byte padding introduced by the compiler into structs could be disastrous when they get read back, so these assertions are very valuable for checking alignment, etc. These should be consistently applied, and in fact this package does a good job with this. As an aesthetic choice it may make sense to put them in a separate detail/*.hxx file; currently this is only done for SourceID. I'll only mention it here and not in the per-file comments, but something to consider.

Unit tests

These are very good; no major issues.

Per-file breakdown

Fragment.hpp

• inline is used at the point of the member function declarations rather than their definitions; not life or death, but typically it's instead put at the definition location as it's considered an implementation detail.
• It's good that the copy and move semantics are made explicit; having said that, they and the destructor should be moved below the "normal" functions (get_header(), etc.) as described here.
• Comments like ///< Fragment copy constructor is deleted aren't needed where the code is self-explanatory
• It may be worth considering dropping the kReadOnlyMode option in that it doesn't seem to be used anywhere in our codebase (outside of unit tests and python bindings). The remaining two enums (kTakeOverBuffer and kCopyFromBuffer) could be generalized, put in Types.hpp, and also used by TriggerRecordHeader (see my comments later)
• The Fragment::Fragment(void* existing_fragment_buffer,... constructor should be explicit about what it means for existing_fragment_buffer to be a nullptr. This could be documenting above the constructor "existing_fragment_buffer as a nullptr is a developer error and will cause a crash", asserting against this, throwing, etc...
• More a judgement call than anything, but it may also be worth considering dropping the Fragment(void* buffer, size_t size); constructor as this is only used in unit tests (dfmodules, hdf5libs) and is simply a special case of Fragment(const std::vector<std::pair<void*, size_t>>& pieces)
• Speaking of Fragment(const std::vector<std::pair<void*, size_t>>& pieces):
  • Could make size be of type fragment_size_t (from Types.hpp)
  • Given how size is calculated, size < sizeof(FragmentHeader) really isn't necessary, and if this is meant to capture overflows it would only capture the tiny fraction which result in a value from 0 to sizeof(FragmentHeader)
  • std::invalid_argument("The Fragment buffer point to NULL."); can be thrown without the memory allocated to m_data_arr being freed up
• set_header_fields: the implementation is both clear about what's happening but vulnerable if new variables/getters are added to the FragmentHeader; an alternative might be to (1) save the size value in the original header, (2) clobber it with the passed header via = assignment, (3) reassign the original size value.
• JCF: need to think about this Does Fragment need a version? Could be a passthrough to the FragmentHeader version.

FragmentHeader.hpp

• Some of the documentation is a bit redundant - e.g., FragmentHeader struct definition at the top of the file, or By default, all status bits are unset. Solutions differ, though - for the former case, maybe a mention/reminder of what the FragmentHeader is, while for the latter, it can just be removed since s_default_status_bits = 0 clearly implies By default, all status bits are unset.
• I can't recall but I would guess this is the result of nomenclature change at some point - "element ID" seems to be an alternate name for "SourceID"? E.g., SourceID element_id;. We could just go with SourceID source_id; (unless an element is a kind of source, in which case a quick comment might be helpful).
• Can remove the commented-out padding
• Any sense in having a detector_id_t, adding it to Types.hpp, and using it in what's currently the uint16_t detector_id; declaration?
• In the FragmentType enum, the comments for kFW_TriggerPrimitive and kTriggerPrimitive are extensive enough that it may be worth considering moving them above the body of the enum, as it makes the enum list harder to read.
• get_fragment_type_names doesn't seem like it needs to be a function since it just wraps a std::map that could be declared in the unnamed namespace {}

Types.hpp

• Should be fine makingTypeDefaults a struct rather than class. In fact, we can go further: these static constexpr defaults could be taken out of a class/struct and could just all be wrapped in a namespace called typedefaults. There wouldn't be a loss of safety; these are all constexpr and you'd just be swapping TypeDefaults::<some default> for typedefaults::<some default>
• Many of the comments are pretty self-explanatory and could be removed ( * @brief Type used to represent run number, etc.)
• * https://github.com/DUNE-DAQ/trgdataformats/blob/develop/include/trgdataformats/TriggerCandidateData.hpp#L22 in comment on trigger_type_t has the correct line, but that #L22 could easily break since daqdataformats doesn't otherwise depend on trgdataformats - just a thought.

TimeSliceHeader.hpp

• Could add a version check to the static_asserts
• Could add an is_in_valid_state function which checks the timeslice_header_marker, whether its member variables (timeslice_number, etc.) have valid values, and whether version matches s_timeslice_header_version

TimeSlice.hpp

• See earlier comment about use of inline in declarations
• copy/move + destructor functions should be moved later in the class declaration and probably don't need redundant comments
• The functionality in set_fragments is only used in dfmodules' TPBundleHandler::get_timeslice function, right after a TimesSlice instance is constructed - perhaps its functionality could be added to a constructor, with its argument as well?
• Could have an is_in_valid_state, which may largely just be a passthrough to the TimeSliceHeader::is_in_valid_state function I suggest above
• In get_total_size_bytes() and get_sum_of_fragment_payload_sizes(), may be worth considering using fragment_size_t here

SourceID.hpp

• Subsystem_t may be overkill, since it's only used in one place, to define the type of the Subsystem enum class
• Use of inline chould be moved from declarations to definitions
• JCF: check this There may be slightly more modern/efficient ways to implement those comparisons (spaceship operator, e.g.)

TriggerRecord.hpp

• TriggerRecord::set_fragments isn't used in our codebase outside of its own unit tests; drop?
• TriggerRecord::get_sum_of_fragment_payload_sizes not used in our codebase outside of its own unit tests and Python bindings; drop?
• TriggerRecord::set_header not used in our codebase outside of its own unit test and (already commented-out) Python bindings; drop?
• copy/move functions and destructor should be repositioned; consider removing some unneeded comments

TriggerRecordHeader.hpp

• In TriggerRecordHeader::TriggerRecordHeader(const std::vector<ComponentRequest>& components), the loop over components at the bottom is unnecessary since C++ guarantees that the elements of a std::vector are contiguous in memory
• TriggerRecordHeader::TriggerRecordHeader(void* existing_trigger_record_header_buffer, bool copy_from_buffer) there are a few improvements which could be made
  • copy_from_buffer, as a boolean, means constructor calls aren't self-documenting (e.g., if I read TriggerRecordHeader(header_buffer, true) it's not immediately clear what true means). A simple enum (kTakeOverBuffer and kCopyFromBuffer) could make this clearer, plus as mentioned earlier be declared in Types.hpp and used in common with Fragment.
  • existing_trigger_record_header_buffer isn't checked for null
  • If TriggerRecordHeaderData were to have an is_in_valid_state function it could be checked before the resulting use of the passed bytes (header->num_requested_components, etc.) - more on this in a moment
• Both at and operator[] are defined, but given that they both do the same thing (bounds-checked access), it seems only one should be retained. Relevant here is that in C++ containers, at is bounds-checked, and operator[] isn't.

TriggerRecordHeaderData.hpp

• As mentioned above, and is_in_valid_state function could be useful. Among other things it could compare trigger_record_header_marker to s_trigger_record_header_magic, version to s_trigger_record_header_version, etc.
• Comments in lines like kUnassigned9 = 9, ///< Status bit 9 is not assigned seem like overkill

fragment.cpp

• Available inFragmentHeader.hpp but lacking bindings: kMPD, get_fragment_type_names, kUnassigned* (see Alessandro's comment), kInvalid
• Available in Fragment.hpp but lacking bindings: all the set_ functions but this is deliberate.