[PATCH] D69546: [SanitizerCommon] Print the current value of options when printing out help.

[Julian Lettner via Phabricator via llvm-commits]

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