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

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka 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 "
----------------
cryptoad wrote:
> 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.
Up to you :-)


================
Comment at: lib/scudo/standalone/flags_parser.cc:48
+
+bool FlagParser::isSpace(char C) {
+  return C == ' ' || C == ',' || C == ':' || C == '\n' || C == '\t' ||
----------------
cryptoad wrote:
> 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.
Only skipWhitespace does not check NULL

So you can either:
```
void FlagParser::skipWhitespace() {
  while (Buffer[Pos] && isSeparator(Buffer[Pos]))
    ++Pos;
}
```
or just add new method
```
bool FlagParser::isSeparatorOrNull(char C) {
  return !C || IsSeparator(C);
}
```

```
// before
(Buffer[Pos] != 0 && Buffer[Pos] != '=' && !isSeparator(Buffer[Pos]))
// after
(Buffer[Pos] != '=' && !isSeparatorOrNull(Buffer[Pos]))

// before
(Buffer[Pos] != 0 && !isSeparator(Buffer[Pos]))
// after
(!isSeparatorOrNull(Buffer[Pos]))

// before
*ValueEnd == 0 || *ValueEnd == '"' || *ValueEnd == '\'' ||  isSeparator(*ValueEnd);
// after
*ValueEnd == '"' || *ValueEnd == '\'' ||  isSeparatorOrNull(*ValueEnd);
```


================
Comment at: lib/scudo/standalone/flags_parser.cc:48
+
+bool FlagParser::isSeparator(char C) {
+  return C == ' ' || C == ',' || C == ':' || C == '\n' || C == '\t' ||
----------------
Not sure why this is in header file.
I'd hide as much as possible into
namespace {} of cc file

BTW. cc is google3, llvm is cpp


================
Comment at: lib/scudo/standalone/flags_parser.h:43
+
+  const char *Buffer;
+  uptr Pos;
----------------
cryptoad wrote:
> 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?
right

```
const char *Buffer = null;
uptr Pos = 0;
```



Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D59597