From 9c77dfe52cb9ab9b92493fd6c55af4e3b2d7ad81 Mon Sep 17 00:00:00 2001 From: David Galiffi Date: Sat, 29 Aug 2026 01:33:51 -0400 Subject: [PATCH 1/3] fix(rocprofiler-systems): resolve clang-tidy findings on pftrace-extension branch - argparse.cpp: derive --output-format max_count from the choices list instead of a hardcoded magic number, so they can't drift apart. - config.cpp: directly include fmt/format.h where fmt::format is used, and rename get_perfetto_output_filename_with_suffix()'s locals to drop the non-conforming leading-underscore style. Co-Authored-By: Claude Sonnet 5 --- .../source/lib/core/argparse.cpp | 6 +- .../source/lib/core/config.cpp | 79 +++++++++++-------- 2 files changed, 49 insertions(+), 36 deletions(-) diff --git a/projects/rocprofiler-systems/source/lib/core/argparse.cpp b/projects/rocprofiler-systems/source/lib/core/argparse.cpp index 0640d8c20042..cc907a9d49ab 100644 --- a/projects/rocprofiler-systems/source/lib/core/argparse.cpp +++ b/projects/rocprofiler-systems/source/lib/core/argparse.cpp @@ -805,6 +805,8 @@ add_core_arguments(parser_t& _parser, parser_data& _data) if(_data.reg.environ_filter("output_format", _data)) { + static const strvec_t k_output_format_choices = { "pftrace", "proto", "rocpd", + "json", "text", "txt" }; _parser .add_argument( { "--output-format" }, @@ -815,9 +817,9 @@ add_core_arguments(parser_t& _parser, parser_data& _data) "combined with --trace, --profile, --flat-profile, or " "--profile-format.") .min_count(1) - .max_count(6) + .max_count(static_cast(k_output_format_choices.size())) .dtype("[format...]") - .choices({ "pftrace", "proto", "rocpd", "json", "text", "txt" }) + .choices(k_output_format_choices) .conflicts( { "trace", "profile", "flat-profile", "profile-format", "use-rocpd" }) .action([&](parser_t& p) { diff --git a/projects/rocprofiler-systems/source/lib/core/config.cpp b/projects/rocprofiler-systems/source/lib/core/config.cpp index 8e14f04f55f8..ff726f527c0a 100644 --- a/projects/rocprofiler-systems/source/lib/core/config.cpp +++ b/projects/rocprofiler-systems/source/lib/core/config.cpp @@ -3114,68 +3114,79 @@ get_output_absolute_path(std::string_view basename, std::string_view extension, return result; } +/** + * Get the Perfetto output filename with a suffix. + * + * @param suffix The suffix to add to the Perfetto output filename. + * @return The Perfetto output filename with the suffix. + */ std::string get_perfetto_output_filename_with_suffix(std::string_view suffix) { - static auto _v = get_config()->find(std::string{ env_vars::PERFETTO_FILE }); - auto _val = static_cast&>(*_v->second).get(); + static auto s_setting = get_config()->find(std::string{ env_vars::PERFETTO_FILE }); + auto val = static_cast&>(*s_setting->second).get(); - LOG_DEBUG("Initial ROCPROFSYS_PERFETTO_FILE='{}', suffix='{}'", _val, suffix); + LOG_DEBUG("Initial ROCPROFSYS_PERFETTO_FILE='{}', suffix='{}'", val, suffix); // If absolute path is provided, return it as-is - if(!_val.empty() && _val.at(0) == '/') + if(!val.empty() && val.at(0) == '/') { - LOG_DEBUG("Absolute path, returning: '{}'", _val); - return _val; + LOG_DEBUG("Absolute path, returning: '{}'", val); + return val; } - auto _pos_dir = _val.find_last_of('/'); - auto _dir = std::string{}; - auto _ext = std::string{ "pftrace" }; + // Parse the filename into directory, basename, and extension + auto pos_dir = val.find_last_of('/'); + auto dir = std::string{}; + auto ext = std::string{ "pftrace" }; - if(_pos_dir != std::string::npos) + // If the filename contains a directory, extract it + if(pos_dir != std::string::npos) { - _dir = _val.substr(0, _pos_dir + 1); - _val = _val.substr(_pos_dir + 1); + dir = val.substr(0, pos_dir + 1); + val = val.substr(pos_dir + 1); } - auto _pos_ext = _val.find_last_of('.'); - if(_pos_ext + 1 < _val.length()) + // Extract the extension + auto pos_ext = val.find_last_of('.'); + if(pos_ext + 1 < val.length()) { - _ext = _val.substr(_pos_ext + 1); - _val = _val.substr(0, _pos_ext); + ext = val.substr(pos_ext + 1); + val = val.substr(0, pos_ext); } // Check if explicitly set via environment OR config file // If explicitly set, don't add suffix; otherwise use provided suffix - bool _explicitly_set = - (_v->second->get_environ_updated() || _v->second->get_config_updated()); + bool explicitly_set = (s_setting->second->get_environ_updated() || + s_setting->second->get_config_updated()); - LOG_DEBUG("Parsed: dir='{}', basename='{}', ext='{}', explicitly_set={}", _dir, _val, - _ext, _explicitly_set); + LOG_DEBUG("Parsed: dir='{}', basename='{}', ext='{}', explicitly_set={}", dir, val, + ext, explicitly_set); LOG_DEBUG("settings::output_path()='{}'", settings::output_path()); - auto _cfg = settings::compose_filename_config{ - !_explicitly_set && !suffix.empty(), // use_suffix only if not explicitly set - suffix, // suffix value - false, // make_dir - _dir // explicit_path + auto cfg = settings::compose_filename_config{ + !explicitly_set && !suffix.empty(), // use_suffix only if not explicitly set + suffix, // suffix value + false, // make_dir + dir // explicit_path }; - _val = settings::compose_output_filename(_val, _ext, _cfg); + val = settings::compose_output_filename(val, ext, cfg); - LOG_DEBUG("After compose_output_filename: '{}'", _val); + LOG_DEBUG("After compose_output_filename: '{}'", val); - if(!_val.empty() && _val.at(0) != '/') + // If the path is relative, prepend the current working directory + if(!val.empty() && val.at(0) != '/') { - auto _result = settings::format(fmt::format("{}/{}", getenv("PWD"), _val), - get_config()->get_tag()); - LOG_DEBUG("Path is relative, prepending PWD: '{}'", _result); - return _result; + auto result = settings::format(fmt::format("{}/{}", getenv("PWD"), val), + get_config()->get_tag()); + LOG_DEBUG("Path is relative, prepending PWD: '{}'", result); + return result; } - LOG_DEBUG("Path is absolute, returning: '{}'", _val); - return _val; + // If the path is absolute, return it as-is + LOG_DEBUG("Path is absolute, returning: '{}'", val); + return val; } std::string From b464a64d79e135d1edcf121e540d082bfa6d4c71 Mon Sep 17 00:00:00 2001 From: David Galiffi Date: Tue, 8 Sep 2026 19:24:14 -0400 Subject: [PATCH 2/3] address clang-tidy findings Add a direct include for --- projects/rocprofiler-systems/source/lib/core/config.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/projects/rocprofiler-systems/source/lib/core/config.cpp b/projects/rocprofiler-systems/source/lib/core/config.cpp index ff726f527c0a..ce6067fe35c5 100644 --- a/projects/rocprofiler-systems/source/lib/core/config.cpp +++ b/projects/rocprofiler-systems/source/lib/core/config.cpp @@ -46,6 +46,7 @@ #include "logger/debug.hpp" +#include #include #include From 27be32bf2a3b3c0c797dcee497796d1fbd093744 Mon Sep 17 00:00:00 2001 From: David Galiffi Date: Tue, 8 Sep 2026 19:35:03 -0400 Subject: [PATCH 3/3] fix(rocprofiler-systems): address Copilot review feedback on perfetto filename helper - Guard the extension-extraction against find_last_of returning npos, which previously wrapped around and clobbered the "pftrace" default extension for extension-less basenames. - Fall back to "." when PWD is unset, matching the pattern already used by sibling helpers in this file, instead of formatting a null C-string. Co-Authored-By: Claude Sonnet 5 --- projects/rocprofiler-systems/source/lib/core/config.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/projects/rocprofiler-systems/source/lib/core/config.cpp b/projects/rocprofiler-systems/source/lib/core/config.cpp index ce6067fe35c5..187e368cd2d1 100644 --- a/projects/rocprofiler-systems/source/lib/core/config.cpp +++ b/projects/rocprofiler-systems/source/lib/core/config.cpp @@ -3150,7 +3150,7 @@ get_perfetto_output_filename_with_suffix(std::string_view suffix) // Extract the extension auto pos_ext = val.find_last_of('.'); - if(pos_ext + 1 < val.length()) + if(pos_ext != std::string::npos && pos_ext + 1 < val.length()) { ext = val.substr(pos_ext + 1); val = val.substr(0, pos_ext); @@ -3179,8 +3179,10 @@ get_perfetto_output_filename_with_suffix(std::string_view suffix) // If the path is relative, prepend the current working directory if(!val.empty() && val.at(0) != '/') { - auto result = settings::format(fmt::format("{}/{}", getenv("PWD"), val), - get_config()->get_tag()); + const auto* pwd = getenv("PWD"); + auto result = settings::format( + fmt::format("{}/{}", (pwd != nullptr && pwd[0] != '\0') ? pwd : ".", val), + get_config()->get_tag()); LOG_DEBUG("Path is relative, prepending PWD: '{}'", result); return result; }