search

nasa / HDTN

High-rate Delay Tolerant Network (HDTN) Software
https://www1.grc.nasa.gov/space/scan/acs/tech-studies/dtn/
Other
91 stars 22 forks source link

Add docs for UdpBatchSender #36

Closed tphnicholas closed 1 year ago

tphnicholas commented 1 year ago

What should the class comment contain and what stays in the description section?

Are the comments descriptive enough?

tphnicholas commented 1 year ago

Continuation from #35

briantomko commented 1 year ago

@tphnicholas These comments in the UdpBatchSenderlook great! I suppose the description and class comments could be identical, unless there's something specific that could be added to the class comment that makes sense.

tphnicholas commented 1 year ago

@briantomko I see, the example in TokenRateLimiter was more of a quick class usage showcase, was unsure if that's what you were going for. Would you like to sort this class comment out while reviewing the rest?

What do you think about documenting @postconditions at least for the public interface? It worked well in this case, not sure how it'll hold for more complex/involved classes, would be open to giving it a try.

briantomko commented 1 year ago

@tphnicholas I would say class comments should be necessary if there are multiple classes in a file, but not otherwise. Also, how were you wanting to do the pull requests (one file per pull or several/all files per pull)? I can review the UdpBatchSender first thing in the morning.

briantomko commented 1 year ago

@tphnicholas Also I concur with documenting @postconditions for just the public interface. Thanks again

tphnicholas commented 1 year ago

@briantomko I was thinking grouping by sub-components for the main network components of DTN should make sense and keep the history kind of clean.

For example starting with 1 PR for the UDP layer of ltp-over-udp which would include the udp engine, ltp-over-udp induct/outduct, the delay simulation utility and this file, thoughts?

tphnicholas commented 1 year ago

And just to keep it visible, found a mismatched comment earlier which functionally has no real impact but does violate the documented invariant:

The call to reserve() here is wrapped by the windows macro: https://github.com/nasa/HDTN/blob/9aec731349d7a21b23eb99702ce5f7a19c5acb44/common/util/src/UdpBatchSender.cpp#L116 And the comment here expects the reserve to have been executed unconditionally: https://github.com/nasa/HDTN/blob/9aec731349d7a21b23eb99702ce5f7a19c5acb44/common/util/src/UdpBatchSender.cpp#L197

Which one should be corrected?

briantomko commented 1 year ago

And just to keep it visible, found a mismatched comment earlier which functionally has no real impact but does violate the documented invariant:

The call to reserve() here is wrapped by the windows macro:

https://github.com/nasa/HDTN/blob/9aec731349d7a21b23eb99702ce5f7a19c5acb44/common/util/src/UdpBatchSender.cpp#L116

And the comment here expects the reserve to have been executed unconditionally: https://github.com/nasa/HDTN/blob/9aec731349d7a21b23eb99702ce5f7a19c5acb44/common/util/src/UdpBatchSender.cpp#L197

Which one should be corrected?

Good catch. That vector gets more pushes for windows (one push per piece of a udp packet) than it does on linux (one push per udp packet). I would suggest reserving 50 for linux and 100 for windows. Could you add a #else reserve 50 and correct the resize comment, and maybe add this as a comment for the reserve part. Feel free to add a static constexpr std::size_t TRANSMIT_PACKETS_RESERVED_SIZE = if that makes it more clear.

briantomko commented 1 year ago

@briantomko I was thinking grouping by sub-components for the main network components of DTN should make sense and keep the history kind of clean.

For example starting with 1 PR for the UDP layer of ltp-over-udp which would include the udp engine, ltp-over-udp induct/outduct, the delay simulation utility and this file, thoughts?

@tphnicholas That sounds good. Just let me know when you feel the PR is ready for merge so I don't accidentally merge it too soon.

tphnicholas commented 1 year ago

@briantomko You could close this one out and I incorporate this file + suggested changes over to a new PR once the rest of the files are ready for review to keep everything bundled together.

briantomko commented 1 year ago

@briantomko You could close this one out and I incorporate this file + suggested changes over to a new PR once the rest of the files are ready for review to keep everything bundled together.

Yes I'll close this out, and the review I did here can go to the new PR/issue.