From 45065658a0b1645f4ca88ba854e2a199ded42200 Mon Sep 17 00:00:00 2001 From: alandefreitas Date: Wed, 2 Oct 2024 13:56:31 -0300 Subject: [PATCH] refactor: split error message from source location in reports fix #672 --- include/mrdocs/Support/Error.hpp | 102 ++++++++++++++++++++----------- src/lib/Lib/CorpusImpl.cpp | 5 +- src/lib/Support/Error.cpp | 30 ++++++--- src/lib/Support/Error.hpp | 3 +- src/tool/ToolMain.cpp | 2 +- util/generate-config-info.py | 34 +++++------ 6 files changed, 109 insertions(+), 67 deletions(-) diff --git a/include/mrdocs/Support/Error.hpp b/include/mrdocs/Support/Error.hpp index 03ad0dd79..10730dd2f 100644 --- a/include/mrdocs/Support/Error.hpp +++ b/include/mrdocs/Support/Error.hpp @@ -2775,7 +2775,62 @@ void print( Level level, std::string const& text, - source_location const* loc = nullptr); + source_location const* loc = nullptr, + Error const* e = nullptr); + +namespace detail { +template +requires (!std::same_as, Error>) +void +log_impl( + Level level, + Located fs, + Arg0&& arg0, + Args&&... args) +{ + std::string str = fmt::vformat( + fs.value, + fmt::make_format_args(arg0, args...)); + return print( + level, + str, + &fs.where); +} + +template +void +log_impl( + Level level, + Located fs, + Error const& e, + Args&&... args) +{ + // When the message is an error, we send split + // the information relevant to the user from + // the information relevant for bug tracking + // so that users can understand the message. + std::string str = fmt::vformat( + fs.value, + fmt::make_format_args(e.reason(), args...)); + return print( + level, + str, + &fs.where, + &e); +} + +inline +void +log_impl( + Level level, + Located fs) +{ + return print( + level, + {}, + &fs.where); +} +} /** Format a message to the console. @@ -2791,17 +2846,15 @@ print( */ template void -format( +log( Level level, Located fs, Args&&... args) { - return print( + return detail::log_impl( level, - fmt::vformat( - fs.value, - fmt::make_format_args(args...)), - &fs.where); + fs, + std::forward(args)...); } /** Report a message to the console. @@ -2812,12 +2865,7 @@ debug( Located format, Args&&... args) { - return print( - Level::debug, - fmt::vformat( - format.value, - fmt::make_format_args(args...)), - &format.where); + return log(Level::debug, format, std::forward(args)...); } /** Report a message to the console. @@ -2828,12 +2876,7 @@ info( Located format, Args&&... args) { - return print( - Level::info, - fmt::vformat( - format.value, - fmt::make_format_args(args...)), - &format.where); + return log(Level::info, format, std::forward(args)...); } /** Report a message to the console. @@ -2844,12 +2887,7 @@ warn( Located format, Args&&... args) { - return print( - Level::warn, - fmt::vformat( - format.value, - fmt::make_format_args(args...)), - &format.where); + return log(Level::warn, format, std::forward(args)...); } /** Report a message to the console. @@ -2860,12 +2898,7 @@ error( Located format, Args&&... args) { - return print( - Level::error, - fmt::vformat( - format.value, - fmt::make_format_args(args...)), - &format.where); + return log(Level::error, format, std::forward(args)...); } /** Report a message to the console. @@ -2876,12 +2909,7 @@ fatal( Located format, Args&&... args) { - return print( - Level::fatal, - fmt::vformat( - format.value, - fmt::make_format_args(args...)), - &format.where); + return log(Level::fatal, format, std::forward(args)...); } } // report diff --git a/src/lib/Lib/CorpusImpl.cpp b/src/lib/Lib/CorpusImpl.cpp index 86e7390a5..905ad75b7 100644 --- a/src/lib/Lib/CorpusImpl.cpp +++ b/src/lib/Lib/CorpusImpl.cpp @@ -187,9 +187,8 @@ build( taskGroup.async( [&, idx = ++index, path = std::move(file)]() { - report::format(reportLevel, + report::log(reportLevel, "[{}/{}] \"{}\"", idx, files.size(), path); - processFile(path); }); } @@ -217,7 +216,7 @@ build( return Unexpected(results.error()); corpus->info_ = std::move(results.value()); - report::format(reportLevel, + report::log(reportLevel, "Extracted {} declarations in {}", corpus->info_.size(), format_duration(clock_type::now() - start_time)); diff --git a/src/lib/Support/Error.cpp b/src/lib/Support/Error.cpp index e8b6c56da..94642b23c 100644 --- a/src/lib/Support/Error.cpp +++ b/src/lib/Support/Error.cpp @@ -10,6 +10,7 @@ #include "lib/Support/Error.hpp" #include +#include #include #include #include @@ -201,13 +202,14 @@ void print( Level level, std::string const& text, - source_location const* loc) + source_location const* loc, + Error const* e) { call_impl(level, [&](llvm::raw_ostream& os) { os << text; - }, loc); + }, loc, e); } //------------------------------------------------ @@ -230,7 +232,8 @@ void call_impl( Level level, std::function f, - source_location const* loc) + source_location const* loc, + Error const* e) { std::string s; if(level >= level_) @@ -242,11 +245,22 @@ call_impl( level == Level::error || level == Level::fatal)) { - os << "\n" << - fmt::format( - " Reported at {}({})", - ::SourceFileNames::getFileName(loc->file_name()), - loc->line()); + os << "\n\n"; + os << "An issue occurred during execution.\n"; + os << "If you believe this is a bug, please report it at https://github.com/cppalliance/mrdocs/issues\n" + "with the following details:\n"; + os << fmt::format(" MrDocs Version: {} (Build: {})\n", project_version, project_version_build); + if (e) + { + os << fmt::format( + " Error Location: `{}` at line {}\n", + ::SourceFileNames::getFileName(e->location().file_name()), + e->location().line()); + } + os << fmt::format( + " Reported From: `{}` at line {}", + ::SourceFileNames::getFileName(loc->file_name()), + loc->line()); // VFALCO attach a stack trace for Level::fatal } os << '\n'; diff --git a/src/lib/Support/Error.hpp b/src/lib/Support/Error.hpp index 81ed49138..ef9ef73ec 100644 --- a/src/lib/Support/Error.hpp +++ b/src/lib/Support/Error.hpp @@ -119,7 +119,8 @@ void call_impl( Level level, std::function f, - source_location const* loc); + source_location const* loc, + Error const* e = nullptr); /** Formatted reporting to a live stream. diff --git a/src/tool/ToolMain.cpp b/src/tool/ToolMain.cpp index 2bc729bb6..2646eb6dc 100644 --- a/src/tool/ToolMain.cpp +++ b/src/tool/ToolMain.cpp @@ -135,7 +135,7 @@ mrdocs_main(int argc, char const** argv) auto exp = DoGenerateAction(configPath, dirs, argv); if (!exp) { - report::error("Generating reference failed: {}", exp.error().message()); + report::error("Generating reference failed: {}", exp.error()); } if (report::results.errorCount > 0 || report::results.fatalCount > 0) diff --git a/util/generate-config-info.py b/util/generate-config-info.py index f57605387..180628449 100644 --- a/util/generate-config-info.py +++ b/util/generate-config-info.py @@ -105,14 +105,14 @@ def validate_and_normalize_option(option): if 'command-line-only' not in option: option['command-line-only'] = False if 'brief' not in option: - raise ValueError(f'Option {option["name"]} must have a "brief" description') + raise ValueError(f'Option "{option["name"]}" must have a "brief" description') if 'details' not in option: option['details'] = '' if 'type' not in option: option['type'] = 'string' if not is_valid_option_type(option['type']): raise ValueError( - f'Option {option["name"]} has an invalid type {option["type"]}: It should be one of {get_valid_option_values()}') + f'Option "{option["name"]}" has an invalid type {option["type"]}: It should be one of {get_valid_option_values()}') if option['type'] == 'enum': if 'values' not in option: raise ValueError(f'Option "{option["name"]}" is of type enum and must have "values"') @@ -541,7 +541,7 @@ def option_validation_snippet(option): validation_contents += f'// s.{camel_name} is required and has no default value\n' validation_contents += f'if (s.{camel_name}.empty())\n' validation_contents += f'{{\n' - validation_contents += f' return Unexpected(formatError("{option["name"]} option is required"));' + validation_contents += f' return Unexpected(formatError("`{option["name"]}` option is required"));' validation_contents += f'}}\n' validation_contents += f'else\n' validation_contents += f'{{\n' @@ -592,17 +592,17 @@ def option_validation_snippet(option): if option['must-exist']: validation_contents += f'if (!s.{camel_name}.empty() && !files::exists(s.{camel_name}))\n' validation_contents += f'{{\n' - validation_contents += f' return Unexpected(formatError("{option["name"]} option: path does not exist: {{}}", s.{camel_name}));\n' + validation_contents += f' return Unexpected(formatError("`{option["name"]}` option: path does not exist: {{}}", s.{camel_name}));\n' validation_contents += f'}}\n' if option['type'] == 'file-path': validation_contents += f'if (files::isDirectory(s.{camel_name}))\n' validation_contents += f'{{\n' - validation_contents += f' return Unexpected(formatError("{option["name"]} option: path should be a regular file: {{}}", s.{camel_name}));\n' + validation_contents += f' return Unexpected(formatError("`{option["name"]}` option: path should be a regular file: {{}}", s.{camel_name}));\n' validation_contents += f'}}\n' if option['type'] == 'dir-path': validation_contents += f'if (!files::isDirectory(s.{camel_name}))\n' validation_contents += f'{{\n' - validation_contents += f' return Unexpected(formatError("{option["name"]} option: path should be a directory: {{}}", s.{camel_name}));\n' + validation_contents += f' return Unexpected(formatError("`{option["name"]}` option: path should be a directory: {{}}", s.{camel_name}));\n' validation_contents += f'}}\n' elif option['type'] in ['file-path', 'dir-path']: validation_contents += f'if (files::exists(s.{camel_name}))\n' @@ -610,12 +610,12 @@ def option_validation_snippet(option): if option['type'] == 'file-path': validation_contents += f'if (files::isDirectory(s.{camel_name}))\n' validation_contents += f'{{\n' - validation_contents += f' return Unexpected(formatError("{option["name"]} option: path should be a regular file: {{}}", s.{camel_name}));\n' + validation_contents += f' return Unexpected(formatError("`{option["name"]}` option: path should be a regular file: {{}}", s.{camel_name}));\n' validation_contents += f'}}\n' if option['type'] == 'dir-path': validation_contents += f'if (!files::isDirectory(s.{camel_name}))\n' validation_contents += f'{{\n' - validation_contents += f' return Unexpected(formatError("{option["name"]} option: path should be a directory: {{}}", s.{camel_name}));\n' + validation_contents += f' return Unexpected(formatError("`{option["name"]}` option: path should be a directory: {{}}", s.{camel_name}));\n' validation_contents += f'}}\n' validation_contents += f'}}\n' @@ -625,7 +625,7 @@ def option_validation_snippet(option): validation_contents += f'// s.{camel_name} paths are required and have no default value\n' validation_contents += f'if (s.{camel_name}.empty())\n' validation_contents += f'{{\n' - validation_contents += f' return Unexpected(formatError("{option["name"]} option is required"));' + validation_contents += f' return Unexpected(formatError("`{option["name"]}` option is required"));' validation_contents += f'}}\n' validation_contents += f'else\n' validation_contents += f'{{\n' @@ -694,7 +694,7 @@ def option_validation_snippet(option): if option['must-exist']: validation_contents += f' if (!files::exists(p))\n' validation_contents += f' {{\n' - validation_contents += f' return Unexpected(formatError("{option["name"]} option: path does not exist: {{}}", p));\n' + validation_contents += f' return Unexpected(formatError("`{option["name"]}` option: path does not exist: {{}}", p));\n' validation_contents += f' }}\n' if option['command-line-sink'] and 'filename-mapping' in option: validation_contents += f' auto f = files::getFileName(p);\n' @@ -724,7 +724,7 @@ def option_validation_snippet(option): validation_contents += f'// s.{camel_name} is required with no default value.' validation_contents += f'if (s.{camel_name}.empty())\n' validation_contents += f'{{\n' - validation_contents += f' return Unexpected(formatError("{option["name"]} option is required"));' + validation_contents += f' return Unexpected(formatError("`{option["name"]}` option is required"));' validation_contents += f'}}\n' else: validation_contents += f'// s.{camel_name} is not required and has no default value\n' @@ -745,7 +745,7 @@ def option_validation_snippet(option): validation_contents += f'// s.{camel_name} is required with no default value.' validation_contents += f'if (s.{camel_name}.empty())\n' validation_contents += f'{{\n' - validation_contents += f' return Unexpected(formatError("{option["name"]} option is required"));' + validation_contents += f' return Unexpected(formatError("`{option["name"]}` option is required"));' validation_contents += f'}}\n' else: validation_contents += f'// s.{camel_name} is not required and has no default value\n' @@ -766,12 +766,12 @@ def option_validation_snippet(option): if 'min-value' in option: validation_contents += f'if (std::cmp_less(s.{camel_name}, {option["min-value"]}))\n' validation_contents += f'{{\n' - validation_contents += f' return Unexpected(formatError("{option["name"]} option: value is less than {option["min-value"]}: {{}}", s.{camel_name}));\n' + validation_contents += f' return Unexpected(formatError("`{option["name"]}` option: value is less than {option["min-value"]}: {{}}", s.{camel_name}));\n' validation_contents += f'}}\n' if 'max-value' in option: validation_contents += f'if (std::cmp_greater(s.{camel_name}, {option["max-value"]}))\n' validation_contents += f'{{\n' - validation_contents += f' return Unexpected(formatError("{option["name"]} option: value is greater than {option["max-value"]}: {{}}", s.{camel_name}));\n' + validation_contents += f' return Unexpected(formatError("`{option["name"]}` option: value is greater than {option["max-value"]}: {{}}", s.{camel_name}));\n' validation_contents += f'}}\n' contents += validation_contents @@ -830,7 +830,7 @@ def generate_public_settings_cpp(config): for option in category['options']: if not option["command-line-only"]: contents += f' // {option["brief"]}\n' - contents += f' io.mapOptional("{option["name"]}", s.{to_camel_case(option["name"])});\n' + contents += f' io.mapOptional("`{option["name"]}`", s.{to_camel_case(option["name"])});\n' contents += ' }\n' contents += '};\n\n' @@ -1060,7 +1060,7 @@ def generate_public_toolargs_cpp(config): option_contents += f' }}\n' option_contents += f' else\n' option_contents += f' {{\n' - option_contents += f' return Unexpected(formatError("{option["name"]} option: invalid value: {{}}", this->{camel_name}));\n' + option_contents += f' return Unexpected(formatError("`{option["name"]}` option: invalid value: {{}}", this->{camel_name}));\n' option_contents += f' }}\n' else: option_contents += f' s.{camel_name} = this->{camel_name};\n' @@ -1149,7 +1149,7 @@ def to_cpp_default_value(option): if option_default.startswith('<'): closing_bracket = option_default.find('>') if closing_bracket == -1: - raise ValueError(f'Invalid default value {option_default} for option {option["name"]}') + raise ValueError(f'Invalid default value {option_default} for option `{option["name"]}`') reference_path = option_default[1:closing_bracket] if reference_path == 'config-dir': option_default = '.' + option_default[closing_bracket + 1:]