[PATCH] D59597: [scudo][standalone] Add flags & related parsers

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad marked 9 inline comments as done.
cryptoad added inline comments.


================
Comment at: lib/scudo/standalone/flags.inc:13
+
+SCUDO_FLAG(int, quarantine_size_kb, 0,
+           "Size (in kilobytes) of quarantine used to delay the actual "
----------------
vitalybuka wrote:
> Maybe more convenient is to add particular flags in patches which use them?
My idea was more the contrary: once a file is checked in, avoid further changes unless something comes up later.
I am not opposed to doing it your way, let me know.


================
Comment at: lib/scudo/standalone/flags_parser.cc:48
+
+bool FlagParser::isSpace(char C) {
+  return C == ' ' || C == ',' || C == ':' || C == '\n' || C == '\t' ||
----------------
vitalybuka wrote:
> isSpace is no exactly checking for space 
> 
> Could we make code simple if we add check for null here?
I renamed this to `isSeparator`, which sounds more appropriate.
As for checking for null, the `parseFlags` loop requires to make a distinction between the whitespace/separators & the null character which will be termination.
So I think it should stay that way, unless I am not seeing something.


================
Comment at: lib/scudo/standalone/flags_parser.h:21
+enum class FlagType : u8 {
+  FT_bool,
+  FT_int,
----------------
morehouse wrote:
> The `FT_` prefix seems redundant since we'll always have to prefix with `FlagType::` anyway.
> 
> I suppose it does make things simpler in `registerFlags` though...
I felt it worked out well with a concatenation macro with the type. It sorts of departs from the casing style, but it allowed to use the type (`bool`, `int`) as is.


================
Comment at: lib/scudo/standalone/flags_parser.h:43
+
+  const char *Buffer;
+  uptr Pos;
----------------
vitalybuka wrote:
> It would be more readable if field are initialized here
I am unclear as to what fields you are referring to, the ones initialized in the constructor?


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D59597