-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Config Printing Updates, main branch (2025.02.04.) #837
Conversation
68e5b05
to
3d4bc82
Compare
Moved them into the traccc::options library, as they will realistically only ever be used there.
3d4bc82
to
2720f30
Compare
|
Before working on a All feedback is welcome! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the changes to the way we return the printables from the interface classes, but the rest of this PR is way, way over-engineered.
Also, I much prefer the previous output format but as you say that's a subjective matter.
|
||
// System include(s). | ||
#include <iostream> | ||
#include <format> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind you that support for std::format
is still all over the place, especially in the Mac world.
@@ -7,7 +7,6 @@ | |||
# Project include(s). | |||
include( traccc-compiler-options-cpp ) | |||
|
|||
add_subdirectory(utils) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a good idea to have a library where stuff that we use across the examples can live, I'd suggest we just keep this.
friend class configuration_category; | ||
friend class configuration_list; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Friend classes are a huge code smell and the code was working perfectly without them before; suggest we refactor this to at least get rid of these friend classes.
|
||
namespace traccc::opts { | ||
|
||
configuration_category::configuration_category(std::string_view name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not seriously splitting up an 80-line header file and a 100-line C++ file into 3 translation units and 4 header files, right? To be frank that's just silly...
using child_type = std::unique_ptr<configuration_printable>; | ||
/// Type for the children of this object | ||
using children_type = std::vector<child_type>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This amount of typedeffing is frivolous and can be removed.
std::string configuration_printable::str() const { | ||
|
||
std::ostringstream out; | ||
print_impl(out, "", "", 0, get_max_key_width_impl()); | ||
return out.str(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not going to fly; when we move to using loggers here we will need to be able to print this configuration line-by-line, and producing a whole string first and then splitting it on newlines is not practical.
Replaced by #845. |
As I said a couple of times, I quite like the formatting of the printouts introduced in #822 by @stephenswat, but I don't really like some of the technicalities in there. So this is my proposal for how I'd prefer to have the code. 🤔
I moved the classes into separate header and source files, and moved them into the
traccc::options
library. Thereby removing thetraccc::utils
library. (I was not a fan of having a separate shared library with just that one header and source file...)I changed the classes to use a generic
std::ostream
internally, and the public API to return anstd::string
with the formatted configuration, instead of printing it itself tostd::cout
.In the client classes I removed a whole lot of unnecessary dynamic casting.
Finally, I changed the printout format a little. Going (as an example) from:
To:
I personally find the latter a little easier on my eyes. But I fully admit that this is a subjective thing. 🤔
Finally, I started using
std::format(...)
in a bunch of places in the code. For instance for printingbool
values. To replace all of the(foo ? "yes" : "no")
code pieces. This is also not a completely objective thing, I admit.