This patch is part of a patch series to add support for FileCheck
numeric expressions. This specific patch extend numeric expression to
support an arbitrary number of operands, either variable or literals.
Copyright:
This patch is part of a patch series to add support for FileCheck
numeric expressions. This specific patch extend numeric expression to
support an arbitrary number of operands, either variable or literals.
Copyright:
Buildable 32945 | |
Build 32944: arc lint + arc unit |
I think I am happy with this, leaving the rest up to James.
llvm/include/llvm/Support/FileCheck.h | ||
---|---|---|
146 | I prefer the description of the return value to give the expected/normal case first, and the error case afterward. So, \returns the expression value, or an error ... |
llvm/include/llvm/Support/FileCheck.h | ||
---|---|---|
513 | I've used it in other place already. I'm using "against" because the diagnostic relates to the buffer held by SM (it is created via SM.GetMessage) | |
llvm/lib/Support/FileCheck.cpp | ||
224 | /There is yes. See patch 9/12 |
Mostly looks good. A few nits still and one or two more significant comments. I think this structure looks much cleaner than the previous version, personally.
llvm/include/llvm/Support/FileCheck.h | ||
---|---|---|
54 | As things stand, this only can represent an unsigned literal. I think you'll want to support negative literals or unary negation operands fairly quickly. Otherwise, the expression "-1" doesn't work. I'm also slightly concerned in the current form that somebody could try to use FileCheckExpressionLiteral for negative numbers, and get surprising results. Perhaps worth updating the comment to say that the class represents an unsigned integer literal. | |
60 | Perhaps "Construct a literal with the specified value." | |
llvm/lib/Support/FileCheck.cpp | ||
65 | Might be worth making this comment more generic e.g. "Handle a failed operation, e.g. due to an undefined variable." That'll make the comment robust to other possible future failure types. | |
278–279 | is always a literal The comment should probably start with "The second..." | |
340 | The comment should probably start with "The first..." | |
736–737 | You've changed this message implying it can have more than one undefined variable, but I don't see any lit test case change that has more than one. You should add/update one. | |
llvm/unittests/Support/FileCheckTest.cpp | ||
17 | I think it would be a good idea to have a test case showing that this can take std::numeric_limits<uint64_t>::max(). | |
25 | const std::set &? | |
30 | Perhaps worth having this print what names are still remaining if not empty. I think you can do something like EXPECT_TRUE(ExpectedUndefVarNames.empty()) << ExpectedUndefVarNames. | |
53 | getValue not getvalue. | |
407 | In other places you've used bool(SubstValue) for this sort of conversion. I think that might be nicer, but don't care that much. Regardless, it should be consistent throughout. | |
541–542 | unique_ptr? |
llvm/include/llvm/Support/FileCheck.h | ||
---|---|---|
513 | Is my explanation satisfying or do you want to use another phrasing? | |
llvm/lib/Support/FileCheck.cpp | ||
736–737 | There was a unit test already but it doesn't test that message indeed. I've added 1 lit test and updated another one, the former for correct substitution and the latter for undefined variables. | |
llvm/unittests/Support/FileCheckTest.cpp | ||
25 | This cannot be const since erase is called on that set. Making it a reference means the set needs to be created in all the caller when now the caller can provide an initializer as is done in expectUndefError below. |
llvm/include/llvm/Support/FileCheck.h | ||
---|---|---|
60 | litteral -> literal | |
513 | Yes, it's fine. Not the phrasing I would use but I don't care much about it. | |
llvm/lib/Support/FileCheck.cpp | ||
65 | eg. -> e.g. | |
llvm/unittests/Support/FileCheckTest.cpp | ||
19 | Why not FileCheckExpressionLiteral Ten(10)? | |
25 | Why not FileCheckExpressionLiteral Max(...)? | |
33 | 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. | |
35 | StringRef? | |
47 | Use std::unordered_set? |
llvm/lib/Support/FileCheck.cpp | ||
---|---|---|
63 | Doesn't look like you need this->. | |
211 | btw it was a bit confusing for me to find there is a method that take bool IsPseudo | |
213 | You can avoid having this temorarily variable: if (ParseVarResult) return parseNumericVariableUse(*ParseVarResult, IsPseudo, SM); But actually it seems you also need to check the result of Expected, | |
458 | I'd suggest adding a comment. | |
llvm/unittests/Support/FileCheckTest.cpp | ||
34 | Seems a different variables naming style used here? Ten, Value vs first, set. You need stick to one I think. |
llvm/lib/Support/FileCheck.cpp | ||
---|---|---|
213 | Oh, sorry, I misread the code, you actually check it. Then you still can avoid having the temp var :) |
llvm/lib/Support/FileCheck.cpp | ||
---|---|---|
211 | 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? | |
llvm/unittests/Support/FileCheckTest.cpp | ||
33 | 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. |
llvm/unittests/Support/FileCheckTest.cpp | ||
---|---|---|
31 | 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. | |
37 | Perhaps make this ", ", and start and end the full string with '{'/'}' to make it easier to read. |
llvm/include/llvm/Support/FileCheck.h | ||
---|---|---|
504–507 | This comment still mentions Expr and SM. | |
llvm/lib/Support/FileCheck.cpp | ||
211 | There is no special naming conventions, but given that parseVariable method always must set IsPseudo flag, |
Address review comments
llvm/lib/Support/FileCheck.cpp | ||
---|---|---|
211 | 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). | |
llvm/unittests/Support/FileCheckTest.cpp | ||
31 | 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. |
llvm/lib/Support/FileCheck.cpp | ||
---|---|---|
211 | You could use a small struct to name the members, perhaps called VariableProperties: struct VariableProperties { StringRef Name; bool IsPseudo; } |
LGTM. Thanks for the hard work on this!
llvm/lib/Support/FileCheck.cpp | ||
---|---|---|
124 | You might be able to just do return {Name, IsPseudo} to avoid the temporary. |
llvm/lib/Support/FileCheck.cpp | ||
---|---|---|
124 | I tried but it didn't work. I think it would if I was returning the structure, but here I'm returning an Expected. As per 6.6.3 C++11 draft: So it tries to build an Expected with that initialization list which fails because I think an Expected<foo> only accepts a Foo. |
llvm/lib/Support/FileCheck.cpp | ||
---|---|---|
124 | Yes, that's it: /home/thomasp/repos/llvm-project/llvm/lib/Support/FileCheck.cpp:107:10: error: no matching constructor for initialization of 'Expected<FileCheckPattern::VariableProperties>' return {Name, IsPseudo}; ^~~~~~~~~~~~~~~~ /home/thomasp/repos/llvm-project/llvm/include/llvm/Support/Error.h:474:3: note: candidate template ignored: requirement 'std::is_convertible<StringRef &, VariableProperties>::value' was not satisfied [with OtherT = llvm::StringRef &] Expected(OtherT &&Val, ^ It tries to convert Name into a VariableProperties and fails. All the other constructors expect an Error or another Expected and fail as well. |
Thanks for your work on this! Looking forward to be able to use these new features the next time I update our fork.
llvm/lib/Support/FileCheck.cpp | ||
---|---|---|
124 | Maybe return VariableProperties{Name, IsPseudo}would work? |
llvm/lib/Support/FileCheck.cpp | ||
---|---|---|
124 | Yes of course. I think you could still do it inline if you want, (not tried, so not 100% certain off the top of my head) and don't mind either way: return VariableProperties {Name, IsPseudo}; |