[PATCH] D69546: [SanitizerCommon] Print the current value of options when printing out help.
Julian Lettner via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Nov 6 09:13:07 PST 2019
yln added a comment.
In D69546#1731024 <https://reviews.llvm.org/D69546#1731024>, @yln wrote:
> In D69546#1730860 <https://reviews.llvm.org/D69546#1730860>, @delcypher wrote:
>
> > I thought that might be ambiguous (Is `...` part of the string or is the string truncated?).
>
>
> Do you anticipate this to be an issue? My feeling is that truncation would only happen with file paths?! I have a slight preference towards the other because it seems simpler (simple, not easy), but I am not blocking on this.
^ Please comment, thanks.
================
Comment at: compiler-rt/lib/msan/msan.cpp:126
+ bool Format(char *buffer, uptr size) final {
+ const char *bool_str = (!(*halt_on_error)) ? "true" : "false";
+ uptr bool_str_len = internal_strlcpy(buffer, bool_str, size);
----------------
> // keep_going is an old name for halt_on_error, and it has inverse meaning.
:(
Maybe flip operands and remove negation to make it easier to follow.
================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h:110-113
+ const char *str_to_use = *t_;
+ if (!str_to_use) {
+ str_to_use = "(nullptr)";
+ }
----------------
`const char *str = *t_ ? *t_ : "(nullptr)";`
More consistent with the other implementations.
================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_flags.cpp:82
explicit FlagHandlerInclude(FlagParser *parser, bool ignore_missing)
- : parser_(parser), ignore_missing_(ignore_missing) {}
+ : parser_(parser), ignore_missing_(ignore_missing), original_path_("") {}
bool Parse(const char *value) final {
----------------
delcypher wrote:
> yln wrote:
> > If I understand correctly, this is a special FlagHandler, that reads a file and sets multiple other options based on the contents of the file.
> >
> > The question is: does it make sense to show the file path as the current value?
> > I would assume that as a result of this parser, that other flags current value are updated.
> > If things get complicated here, I think it would be good enough for now to show nothing or some string "<options from file>".
> >
> > Can we add a test for this parser type?
> I think it makes sense to show the path to where the options were read from. I would prefer if we showed the versions where the substitutions were expanded but I refrained from doing that here because that would require us to not free the mmaped memory.
>
> The test is `options-help-include-current-value.cpp`.
Makes sense.
================
Comment at: compiler-rt/test/sanitizer_common/TestCases/options-help.cpp:5
int main() {
}
----------------
I think we dropped the test for truncation?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69546/new/
https://reviews.llvm.org/D69546
More information about the llvm-commits
mailing list