bryancall commented on code in PR #13686:
URL: https://github.com/apache/trafficserver/pull/13686#discussion_r4017603540
##########
src/mgmt/rpc/server/unit_tests/test_rpcserver.cc:
##########
@@ -106,8 +106,10 @@ DbgCtl dbg_ctl{"rpc.test.client"};
bool
try_setup_rpc_test_paths(fs::path const &base, std::string &error)
{
- auto const dir_template = (base / rpc_test_dir_template).string();
- auto const socket_path = (fs::path{dir_template} /
rpc_test_socket_name).string();
+ fs::path const template_path = base / rpc_test_dir_template;
Review Comment:
I think this one is already satisfied on master, so no change made — please
double-check me. `test_rpcserver.cc` has `#include "swoc/swoc_file.h"` at line
47 and its own `namespace fs = swoc::file;` at line 64, both pre-existing (they
are on master, not introduced by this PR). So the alias is local and the
libswoc header is the one included; there is no dependency on
`http/remap/PluginDso.h` here. The commit in this PR only changed two `auto`
copies into `std::string const &` and did not touch includes or the alias. If
you were looking at a different symbol, point me at it and I will fix it.
##########
src/proxy/http/remap/unit-tests/test_PluginFactory.cc:
##########
@@ -395,7 +395,7 @@ SCENARIO("loading plugins", "[plugin][core]")
WHEN("config using nonexisting absolute plugin file name")
{
- fs::path relativeExistingPath = pluginName;
+ fs::path relativeExistingPath = std::move(pluginName);
Review Comment:
I would rather not, and the reason is specific to how Coverity reads this
file. `pluginName` is declared at line 259 in the `GIVEN` and read by seven
`WHEN` blocks (264, 282, 300, 320, 343, 375, 398). Catch2 re-runs the enclosing
body per leaf section, so at runtime a move in any single `WHEN` is safe — but
Coverity does not model that re-entry. It reads the body as straight-line code,
in which line 398 is the last read of `pluginName` and every earlier site is
followed by further reads. That is exactly why it flagged 398 and left 264 and
375 alone.
So moving at the earlier sites would trade one closed COPY_INSTEAD_OF_MOVE
for several new USE_AFTER_MOVE reports, and it would also make each of those
lines depend on Catch2 section semantics for correctness rather than being
locally obvious. If you want the consistency anyway I will do it, but I think
the asymmetry here is load-bearing rather than an oversight.
##########
src/records/unit_tests/test_ConfigRegistry.cc:
##########
@@ -30,7 +30,7 @@ using config::ConfigRegistry;
using config::ConfigSource;
// Shared no-op handler for test registrations
-static config::ConfigReloadHandler noop_handler = [](ConfigContext) {};
+static config::ConfigReloadHandler noop_handler = [](ConfigContext const &) {};
Review Comment:
It works because the call signature on the `std::function` governs
invocation, not the callable. `std::function<void(ConfigContext)>` materializes
a `ConfigContext` by value and then invokes the target with it; a lambda whose
parameter is `ConfigContext const &` is invocable with that argument, so
`std::is_invocable_v` is satisfied and the assignment compiles. I verified the
construction specifically rather than assuming it.
What it does and does not buy is worth being clear about. It removes the
copy at the lambda boundary, which is the site Coverity flagged (`_FUN` and
`operator()` are the same captureless lambda, hence the two CIDs). It does not
remove the copy `std::function` itself makes, because the typedef at
`include/mgmt/config/ConfigRegistry.h:65` takes `ConfigContext` by value and
the comment above it says that is deliberate. If Coverity keeps flagging this
family the real fix is that typedef, not the tests, and production callers such
as `src/api/InkAPI.cc:3482` have the same shape. That is out of scope for a
test-only PR.
--
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]
