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

[Thomas Preud'homme via Phabricator via llvm-commits]

thopre added inline comments.


================
Comment at: llvm/lib/Support/FileCheck.cpp:199
+    bool IsPseudo;
+    Expected<StringRef> ParseVarResult = parseVariable(Expr, IsPseudo, SM);
+    if (ParseVarResult) {
----------------
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).


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:32
+
+StringRef toString(const std::unordered_set<std::string> &Set) {
+  bool First = true;
----------------
jhenderson wrote:
> static?
> 
> Though saying that, this is inside an anonymous namespace, so perhaps the other functions need static removing.
> 
> This function returns a `StringRef`, yet the base type is a local variable. I think that will result in the memory being freed under the reference. Perhaps better would be to make Str a Twine and then use Twine::str() to return a std::string.
Ah yes indeed, the StringRef does not own the string it references. I cannot use a Twine for Str because you cannot store a Twine so while I can do Str + S, I cannot store the result in Str (see https://llvm.org/doxygen/classllvm_1_1Twine.html#a28b850ffd65d5997cf730d644e06d89b I've been bitten by this before in this patchset). I guess I'll return a std::string then. I've used a Twine to do the " " concatenation though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60387