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

[Julian Lettner via Phabricator via llvm-commits]

yln added a comment.

Thanks for this patch, I am pretty sure I will find this useful!  :)



================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.cpp:69
+    else
+      Printf("\t%s\n\t\t- %s\n", flags_[i].name, flags_[i].desc);
+  }
----------------
delcypher wrote:
> eugenis wrote:
> > How about printing a truncated value in this case, maybe replacing the last few characters with dots (...)? At least for string flags.
> > 
> In which case? Currently `success` being false is meant to indicate any type of failure (e.g. the current value is a nullptr which we can't print). A failure doesn't imply that truncation happened. Currently I interpret failure to mean that the buffer should not be read from because it's not in a defined state.
> 
> We could change the semantics to be that failure means truncation if that's what you want. At this point in the code though we don't know what the type of the flag is though so we can't make a string vs not string distinction.
> 
> I'd prefer to keep the current semantics of failure and just change `FlagHandler<const char *>::CurrentValueAsString(...)` to replace the last few characters with `...` in the case of truncation.
+1 for truncation.  I think we can always print a value and never fail.

Completely unnecessary, but for bonus points we could do the following.

With a `buffer[n+4]`: `<n/2 characters>...<n/2 characters>`:
`/super/long/path/to/something` -> `/super/lo...mething`

Or even smarter, with some heuristic for path components:
`/super/long/path/to/something` -> `/super/.../to/something`

This would be just "nice to have".  I am fine with just truncating at the end.




================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h:25
   virtual bool Parse(const char *value) { return false; }
+  virtual bool CurrentValueAsString(char *buffer, uptr size) { return false; }
 
----------------
Parsing can fail, but formatting a correct (truncated) value into a buffer should always be possible.  So I think the return value of this can be void.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h:78
+    str_to_use = reinterpret_cast<const char *>(false_str);
+  }
+  if (size < copy_count)
----------------
delcypher wrote:
> eugenis wrote:
> > const char *str = *t_ ? "true" : "false;
> > uptr copy_count = internal_strnlen(str, size);
> > 
> That's a lot more concise. I'll go with that.
+1


================
Comment at: compiler-rt/test/sanitizer_common/TestCases/options-help-current-values.cpp:4
+
+int main() {
+}
----------------
Can you extend the test to show that we display the current value, i.e, in separate `RUN` and `CHECK` lines provide some option and assert that the printed value is different from the default.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69546/new/

https://reviews.llvm.org/D69546