[PATCH] D49084: FileCheck: Add support for numeric variables and expressions
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Jan 4 07:48:10 PST 2019
jhenderson added a comment.
This change is so large that it's going to take quite a while to review. However, I've made a few initial comments on the header. I'll come back to this next week and continue from there.
================
Comment at: include/llvm/Support/FileCheck.h:41
+//===----------------------------------------------------------------------===//
+// Numeric expression handling Code.
+//===----------------------------------------------------------------------===//
----------------
Nit: Code -> code
================
Comment at: include/llvm/Support/FileCheck.h:44
+
+struct FileCheckNumExprFmtType;
+
----------------
Why the declaration here?
================
Comment at: include/llvm/Support/FileCheck.h:52
+ unsigned Signed : 1;
+ /// Value should be print as hex number.
+ unsigned Hex : 1;
----------------
Nit: print -> printed
================
Comment at: include/llvm/Support/FileCheck.h:57
+
+ /// When 0, absence of format, eg. for literals in an expression.
+ unsigned Set : 1;
----------------
I don't really understand this comment. I think you are saying that this "Set" variable indicates whether the format is explicit or not, right? If so, rename the variable to something like IsExplicit or similar.
================
Comment at: include/llvm/Support/FileCheck.h:60
+
+ /// Several conflicting implicit formats in an expression.
+ unsigned Conflict : 1;
----------------
Several -> If set, there are ...
================
Comment at: include/llvm/Support/FileCheck.h:81
+
+namespace Check {
+/// Phases of FileCheck. Used to determine at what phase is a numeric
----------------
Don't namespace this. Just make the enum inside a scoped enum (i.e. `enum class`).
================
Comment at: include/llvm/Support/FileCheck.h:82-84
+/// Phases of FileCheck. Used to determine at what phase is a numeric
+/// expression (and thus value of any potential variable defined by it)
+/// available.
----------------
I think the general style in LLVM is single space after full stop (applies here and elsewhere).
"is a numeric expression (and thus value ...) available" -> "a numeric expression is available (and thus the value ...)"
================
Comment at: include/llvm/Support/FileCheck.h:95-96
+
+/// Class representing a numeric expression's value. Used to be able to store
+/// and return both signed and unsigned value.
+class FileCheckNumExprVal {
----------------
I'm not sure the second sentence here is necessary.
================
Comment at: include/llvm/Support/FileCheck.h:97
+/// and return both signed and unsigned value.
+class FileCheckNumExprVal {
+private:
----------------
I'm not sure you need to name all your classes "FileCheckXXX". You can shorten their names by dropping the FileCheck part, I reckon.
================
Comment at: include/llvm/Support/FileCheck.h:99-103
+ /// Signed value.
+ int64_t SVal;
+
+ /// Unsigned value.
+ uint64_t UVal;
----------------
Why not just always store in a single uint64_t field called `Value` and convert to signed when requested?
Also, don't unnecessarily abbreviate variables like this.
================
Comment at: include/llvm/Support/FileCheck.h:131-132
+
+ /// Define how to compare value. Used to decide whether a matched numeric
+ /// value satisfies its corresponding numeric expression.
+ bool operator==(const FileCheckNumExprVal &other);
----------------
I don't think you need this comment. operator== is fairly self-explanatory as to its purpose...!
================
Comment at: include/llvm/Support/FileCheck.h:139
+ /// Whether value is signed.
+ bool IsSigned() const { return Signed; }
+
----------------
LLVM style is for camel-case, starting with lower-case letters for method and function names. Also, don't bother commenting if the function purpose is obvious from its name.
================
Comment at: include/llvm/Support/FileCheck.h:145
+ /// Whether value is tentative.
+ bool IsTentative() const { return Tentative; }
+
----------------
What is a tentative value?
================
Comment at: include/llvm/Support/FileCheck.h:169
+ /// representation (true if value is invalid).
+ bool GetStringRepr(struct FileCheckNumExprFmtType Fmt,
+ std::string &StringRepr) const;
----------------
jhenderson wrote:
> This should probably return an Expected<std::string> or Optional<std::string> and not use an output parameter.
You don't need the "struct" here or in any of the other function arguments or variable declarations etc.
================
Comment at: include/llvm/Support/FileCheck.h:169-170
+ /// representation (true if value is invalid).
+ bool GetStringRepr(struct FileCheckNumExprFmtType Fmt,
+ std::string &StringRepr) const;
+
----------------
This should probably return an Expected<std::string> or Optional<std::string> and not use an output parameter.
================
Comment at: include/llvm/Support/FileCheck.h:172-180
+ /// Perform an addition operation. Return an invalid value in case of
+ /// underflow or overflow.
+ static FileCheckNumExprVal Add(const FileCheckNumExprVal &Op1,
+ const FileCheckNumExprVal &Op2);
+
+ /// Perform a substraction operation. Return an invalid value in case of
+ /// underflow or overflow.
----------------
Could these two just be overloads of `operator+` and `operator-`?
================
Comment at: include/llvm/Support/FileCheck.h:183-184
+
+/// Base class representing the AST of a given numeric expression. Only used
+/// for AST node of unknown type. All actual nodes are of another type.
+class FileCheckNumExprAST {
----------------
Only the first sentence here is needed. The other two are implicit by the fact that this is a virtual class with abstract methods.
================
Comment at: include/llvm/Support/FileCheck.h:191-192
+ /// \p Fmt the implicit conversion that applies from any variable used or
+ /// null if no variable is used by the expression. This method must be
+ /// overrided in all subclasses.
+ virtual FileCheckNumExprVal Eval(struct FileCheckNumExprFmtType &Fmt) = 0;
----------------
What does "null" mean in this context? We're not dealing with pointers... Assuming it's some kind of no-format, perhaps calling it by the corresponding variable name might be better.
Again, the last sentence is unnecessary, since it's implicit in the "= 0" part of the function declaration.
================
Comment at: include/llvm/Support/FileCheck.h:195-198
+ /// Append names of undefined variables used in the expression represented by
+ /// this AST. Must be overrided in any subclass representing an expression
+ /// that can contain a variable.
+ virtual void GetUndefVarNames(std::vector<StringRef> &UndefVarNames) const {}
----------------
This isn't the behaviour I'd expect a function of this name to do. I'd expect it to return a vector of names, not append to an input parameter.
Other nit in the comment: overrided -> overridden.
================
Comment at: include/llvm/Support/FileCheck.h:201
+
+/// Class representing a litteral, either in the AST of a numeric expression or
+/// in the result of the evaluation of such an expression.
----------------
Nit: litteral -> literal
================
Comment at: include/llvm/Support/FileCheck.h:209-213
+ /// Constructor for a signed literal.
+ FileCheckNumExprLiteral(int64_t Val) : Value(Val) {}
+
+ /// Constructor for an unsigned literal.
+ FileCheckNumExprLiteral(uint64_t Val) : Value(Val) {}
----------------
Similar to the FileCheckNumExprVal class, I don't think you need both constructors. One should suffice.
================
Comment at: include/llvm/Support/FileCheck.h:215-217
+ /// Evaluate the value of this literal. Therefore returns this node itself
+ /// and set \p Fmt to null since literals do not carry any implicit
+ /// conversion.
----------------
I'd drop the first sentence and rephrase the second:
"Return the node itself and set \p Fmt to none, since literals do not have conversion formats" (where none is a placeholder for a better word than null).
================
Comment at: include/llvm/Support/FileCheck.h:227-228
+
+ /// Value of the numeric expression. Set either from evaluating the AST if
+ /// any and using only variable defined on previous CHECK line, or from the
+ /// matched value if constraint is satisfied.
----------------
from x 2 -> by
================
Comment at: include/llvm/Support/FileCheck.h:232
+
+ /// Matching format.
+ struct FileCheckNumExprFmtType Fmt;
----------------
It's a little unclear what is meant by "matching format" here. Is it the format this expression can match against (i.e. hex/decimal etc), or what?
================
Comment at: include/llvm/Support/FileCheck.h:236
+ /// FileCheck phase when this numeric expression can be evaluated.
+ enum Check::FileCheckPhase KnownPhase;
+
----------------
Like `struct`, you don't nee the `enum` here.
================
Comment at: include/llvm/Support/FileCheck.h:246
+
+ /// Constructor for numeric expression with a known value in parse phase,
+ /// eg. the numeric expression defining the @LINE numeric variable (and
----------------
for numeric -> for a numeric
================
Comment at: include/llvm/Support/FileCheck.h:247-248
+ /// Constructor for numeric expression with a known value in parse phase,
+ /// eg. the numeric expression defining the @LINE numeric variable (and
+ /// currently only used for that).
+ FileCheckNumExpr(FileCheckNumExprVal Value,
----------------
eg. -> e.g.
I'd drop the bit in parentheses at the end of the sentence. If somebody starts using this for another person, this comment will almost certainly go stale.
================
Comment at: include/llvm/Support/FileCheck.h:252-253
+
+ /// Return pointer to AST of the numeric expression. Pointed-to AST is
+ /// managed by a shared_ptr and is guaranteed live as long as this object is.
+ FileCheckNumExprAST *GetAST() const { return AST.get(); }
----------------
Drop the reference to the shared_ptr in this sentence, and change the second sentence to something like "The pointer is guaranteed to be valid as long as this object is."
================
Comment at: include/llvm/Support/FileCheck.h:256
+
+ /// Return the matched value for the expression.
+ FileCheckNumExprVal GetValue() const { return Value; }
----------------
What is meant by "matched" in this context?
================
Comment at: include/llvm/Support/FileCheck.h:268
+
+ /// Return at what FileCheck phase can this expression can be evaluated.
+ enum Check::FileCheckPhase GetKnownPhase() { return KnownPhase; }
----------------
Remove the first "can"
================
Comment at: include/llvm/Support/FileCheck.h:274
+ /// Return whether \p StrVal correspond to a valid and representable value.
+ bool ValueFromStringRepr(StringRef StrVal, FileCheckNumExprVal &Val) const;
+
----------------
Rather than returning a bool, and passing in a parameter that will be set, this and similar functions should probably return an Expected<T> or Optional<T>.
================
Comment at: include/llvm/Support/FileCheck.h:294-299
+ /// Store in \p EvaluatedValue the value evaluated from the constraint of
+ /// this numeric expression. Return whether evaluation failed.
+ bool Eval(FileCheckNumExprVal &EvaluatedValue) const;
+
+ /// Set Value from the result of calling Eval(). Return whether it failed.
+ bool SetValueFromEval();
----------------
Why are these two different functions?
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D49084/new/
https://reviews.llvm.org/D49084
More information about the llvm-commits
mailing list