Skip to content

Commit

Permalink
refactor: split error message from source location in reports
Browse files Browse the repository at this point in the history 
fix #672
  • Loading branch information
@alandefreitas
alandefreitas committed Oct 4, 2024
1 parent f3b33a4 commit 4506565
Show file tree
Hide file tree
Showing 6 changed files with 109 additions and 67 deletions.
102 changes: 65 additions & 37 deletions include/mrdocs/Support/Error.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<class Arg0, class... Args>
requires (!std::same_as<std::decay_t<Arg0>, Error>)
void
log_impl(
Level level,
Located<std::string_view> 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<class... Args>
void
log_impl(
Level level,
Located<std::string_view> 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<std::string_view> fs)
{
return print(
level,
{},
&fs.where);
}
}

/** Format a message to the console.
Expand All @@ -2791,17 +2846,15 @@ print(
*/
template<class... Args>
void
format(
log(
Level level,
Located<std::string_view> 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>(args)...);
}

/** Report a message to the console.
Expand All @@ -2812,12 +2865,7 @@ debug(
Located<std::string_view> 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>(args)...);
}

/** Report a message to the console.
Expand All @@ -2828,12 +2876,7 @@ info(
Located<std::string_view> 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>(args)...);
}

/** Report a message to the console.
Expand All @@ -2844,12 +2887,7 @@ warn(
Located<std::string_view> 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>(args)...);
}

/** Report a message to the console.
Expand All @@ -2860,12 +2898,7 @@ error(
Located<std::string_view> 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>(args)...);
}

/** Report a message to the console.
Expand All @@ -2876,12 +2909,7 @@ fatal(
Located<std::string_view> 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>(args)...);
}

} // report
Expand Down
5 changes: 2 additions & 3 deletions src/lib/Lib/CorpusImpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
}
Expand Down Expand Up @@ -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));
Expand Down
30 changes: 22 additions & 8 deletions src/lib/Support/Error.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

#include "lib/Support/Error.hpp"
#include <mrdocs/Support/Path.hpp>
#include <mrdocs/Version.hpp>
#include <llvm/Support/Mutex.h>
#include <llvm/Support/raw_ostream.h>
#include <llvm/Support/Signals.h>
Expand Down Expand Up @@ -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);
}

//------------------------------------------------
Expand All @@ -230,7 +232,8 @@ void
call_impl(
Level level,
std::function<void(llvm::raw_ostream&)> f,
source_location const* loc)
source_location const* loc,
Error const* e)
{
std::string s;
if(level >= level_)
Expand All @@ -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';
Expand Down
3 changes: 2 additions & 1 deletion src/lib/Support/Error.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,8 @@ void
call_impl(
Level level,
std::function<void(llvm::raw_ostream&)> f,
source_location const* loc);
source_location const* loc,
Error const* e = nullptr);

/** Formatted reporting to a live stream.
Expand Down
2 changes: 1 addition & 1 deletion src/tool/ToolMain.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
34 changes: 17 additions & 17 deletions util/generate-config-info.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"')
Expand Down Expand Up @@ -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'
Expand Down Expand Up @@ -592,30 +592,30 @@ 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'
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'
validation_contents += f'}}\n'

Expand All @@ -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'
Expand Down Expand Up @@ -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'
Expand Down Expand Up @@ -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'
Expand All @@ -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'
Expand All @@ -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
Expand Down Expand Up @@ -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'

Expand Down Expand Up @@ -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'
Expand Down Expand Up @@ -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:]
Expand Down

0 comments on commit 4506565

Please sign in to comment.