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

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

thopre marked 16 inline comments as done.
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:
> 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?


================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:33
+namespace std {
+std::ostream &operator<<(std::ostream &os, const std::set<std::string> &set) {
+  bool first = true;
----------------
jhenderson wrote:
> Other instances of overloading `operator<<` in the LLVM unit tests tend to just be in the llvm namespace, not in std. See for example ADT/StringRefTest.cpp.
> 
> The variables in this function don't use LLVM naming style.
I'm not sure why but when I try with llvm namespace and I switch to unordered_set as suggested below the overloaded operator does not get used and the code fails to compile. However the code compiles and works with llvm namespace if I use a set instead of unordered_set.

I've thus decided to just write a setToString method and output the result of calling it. It does create lots of temporaries though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60387