[PATCH] D60387: FileCheck [7/12]: Arbitrary long numeric expressions

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Support/FileCheck.cpp:199
+    bool IsPseudo;
+    Expected<StringRef> ParseVarResult = parseVariable(Expr, IsPseudo, SM);
+    if (ParseVarResult) {
----------------
thopre wrote:
> grimar wrote:
> > thopre wrote:
> > > grimar wrote:
> > > > btw it was a bit confusing for me to find there is a method that take `bool IsPseudo`
> > > > and method that take `bool &IsPseudo`, i.e. that one of them can change this flag, but another - not.
> > > You mean parseVariable Vs parseNumericVariableUse? Would you rather me have both be reference even though the latter only needs to get the value? Or is there a naming convention to denote a reference as opposed to a parameter passed by value?
> > There is no special naming conventions, but given that `parseVariable` method always must set `IsPseudo` flag,
> > but another `parse*` methods can only read the flags they are given, I wonder if it be better/cleaner for `parseVariable` to return `Expected<std::pair<StringRef, bool>` for example?
> > 
> > 
> I'm not a big fan of pairs because its members are accessed without a name (first or second) which I find it less readable. Anyway, done but I've reverted the change to make the call to parseNumericVariableUse a one liner so that it is clear what is being passed to that function (ParseVarResult->first is not very descriptive).
You could use a small struct to name the members, perhaps called VariableProperties:

```
struct VariableProperties {
  StringRef Name;
  bool IsPseudo;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60387