[PATCH] D60387: FileCheck [7/12]: Arbitrary long numeric expressions
George Rimar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Jul 12 02:16:00 PDT 2019
grimar added inline comments.
================
Comment at: llvm/include/llvm/Support/FileCheck.h:580
/// the class instance representing that variable if successful, or an error
/// holding a diagnostic against \p SM otherwise.
+ Expected<std::unique_ptr<FileCheckNumericVariableUse>>
----------------
This comment still mentions `Expr` and `SM`.
================
Comment at: llvm/lib/Support/FileCheck.cpp:199
+ bool IsPseudo;
+ Expected<StringRef> ParseVarResult = parseVariable(Expr, IsPseudo, SM);
+ if (ParseVarResult) {
----------------
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?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60387/new/
https://reviews.llvm.org/D60387
More information about the llvm-commits
mailing list