brbzull0 commented on code in PR #13904:
URL: https://github.com/apache/trafficserver/pull/13904#discussion_r4376130156
##########
src/mgmt/config/ConfigContext.cc:
##########
@@ -27,9 +27,20 @@
#include <yaml-cpp/yaml.h>
-// Defined here (not = default in header) so that YAML::Node ctor/dtor/copy
-// symbols are only emitted in this TU (part of librecords, which links
yaml-cpp).
-// Otherwise every consumer of RecCore.h would need yaml-cpp at link time.
+#include <optional>
+
+// Shared by every copy of a context, so a built Payload is never changed: the
+// setters build a new one instead, because assigning into a YAML::Node writes
+// through to every other handle on the same node. The YAML it holds is still
+// shared, so a write through a returned node reaches every copy.
+struct ConfigContext::Payload {
+ std::optional<YAML::Node> supplied_yaml;
+ std::optional<YAML::Node> reload_directives;
+};
+
+// ConfigContext is passed by value to every reload handler.
+static_assert(sizeof(ConfigContext) <= 64, "ConfigContext is passed by value;
add new state to ConfigContext::Payload");
+
Review Comment:
Moved the four defaulted special members into the header. With the YAML in
`Payload`, none of them touches `YAML::Node` or needs `Payload` complete, so
there was no reason left to keep them out of line. The copy members are still
user-declared, so move stays suppressed, and a new test in
`test_ReloadDirectives.cc` checks that `std::move(ctx)` still leaves the source
valid.
##########
src/mgmt/config/ConfigContext.cc:
##########
@@ -27,9 +27,20 @@
#include <yaml-cpp/yaml.h>
-// Defined here (not = default in header) so that YAML::Node ctor/dtor/copy
-// symbols are only emitted in this TU (part of librecords, which links
yaml-cpp).
-// Otherwise every consumer of RecCore.h would need yaml-cpp at link time.
+#include <optional>
+
+// Shared by every copy of a context, so a built Payload is never changed: the
+// setters build a new one instead, because assigning into a YAML::Node writes
+// through to every other handle on the same node. The YAML it holds is still
+// shared, so a write through a returned node reaches every copy.
+struct ConfigContext::Payload {
+ std::optional<YAML::Node> supplied_yaml;
+ std::optional<YAML::Node> reload_directives;
+};
+
+// ConfigContext is passed by value to every reload handler.
+static_assert(sizeof(ConfigContext) <= 64, "ConfigContext is passed by value;
add new state to ConfigContext::Payload");
Review Comment:
This comment is cut off after "The bound of". If it is about the 64 in the
`static_assert`: the comment above it now says why. Coverity flags by-value
parameters over 128 bytes, and 64 keeps the class well under that with
libstdc++ and libc++, where it is 32.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
