Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
A work in progress. Contains changes that we likely want to see addressed separately.
Code change looks good to me. Are the TODO cases where the test fails if changing them?
llvm/test/FileCheck/missspelled-directive.txt | ||
---|---|---|
19 | What about P4-COUNT_2? Is that case not really related? |
Yes, this and where the fix is not perfectly obvious.
llvm/test/FileCheck/missspelled-directive.txt | ||
---|---|---|
19 | Yeah, that's intentionally left for later. At the moment I don't even know how severe the problem with COUNT_* is, need to try and see. |
llvm/lib/FileCheck/FileCheck.cpp | ||
---|---|---|
1774–1781 | Instead of introducing a new wrapper, why don't you change all the return to call a constructor method (e.g. make_check_type()) that does what this wrapper do? Then there would not be any FindCheckType that take a Misspelled parameter. I'm also not sure about Misspelled being a check kind. It feels conceptually wrong but on the other hand I guess it makes the implementation simpler. |
llvm/lib/FileCheck/FileCheck.cpp | ||
---|---|---|
1774–1781 | Tried that. Replacing the returned pair with a new CheckLine kind of object implementing the misspelled-related logic seems to add a lot of extra clutter such as the definition of the new structure itself, but especially all the repetitive mentions of Misspelled on every return. Feels like having it as a reference parameter works better, as we only need to alter the flag occasionally. Regarding CheckMisspelled, now that we have CheckBadNot and CheckBadCount, this looks the usual way of propagating the information about our spelling-related concerns. Might be not the best design and may be worth looking into at some point, but at least doesn' seem to be specific to this patch? |
llvm/lib/FileCheck/FileCheck.cpp | ||
---|---|---|
1774–1781 | I was thinking something along the line of: return getRealCheckType(CHECK::CheckBadCount, Rest, Misspelled); with: static std::pair<Check::FileCheckType, StringRef> getRealCheckType(Check::FileCheckType CheckType, StringRef Rest, bool Misspelled) { if (CheckType != Check::CheckNone && Misspelled) return {Check::CheckMisspelled, Rest}; return {CheckType, Rest}; } Fair enough for CheckMisspelled, there is indeeed precedent. |
llvm/lib/FileCheck/FileCheck.cpp | ||
---|---|---|
1774–1781 | That unfortunately wouldn't eliminate the repetitive return getRealCheckType(..., Misspelled) bits, thus adding a significant amount of clutter -- all for the sake of a single assignment where we raise the flag, while also making the code more fragile as the compiler wouldn't then be able to catch returns without calling getRealCheckType(). And if that's not enough, then the name of the function sounds like we introduce one of the most irritating kinds of concepts -- the 'real' ones. :-) |
llvm/lib/FileCheck/FileCheck.cpp | ||
---|---|---|
1774–1781 | Fair enough. LGTM for the FileCheck part then. Have you sent a message to discourse to ask test authors for help on the TODO? |
llvm/lib/FileCheck/FileCheck.cpp | ||
---|---|---|
1774–1781 |
llvm/test/DebugInfo/X86/debug-info-template-parameter.ll | ||
---|---|---|
36 ↗ | (On Diff #430322) | Should be fixed by rGf718664866ab |
Tagging more people in hope to add visibility.
clang/test/CodeGen/cmse-clear-return.c | ||
---|---|---|
235 ↗ | (On Diff #430322) | |
llvm/test/CodeGen/AArch64/fp16-v8-instructions.ll | ||
682–688 ↗ | (On Diff #430322) | |
llvm/test/CodeGen/AMDGPU/phi-vgpr-input-moveimm.mir | ||
3–5 ↗ | (On Diff #430322) | |
llvm/test/CodeGen/Thumb2/thumb2-execute-only-prologue.ll | ||
13 ↗ | (On Diff #430322) | |
llvm/test/CodeGen/WebAssembly/libcalls.ll | ||
52–54 | ||
llvm/test/CodeGen/X86/GlobalISel/select-ext.mir | ||
69–72 ↗ | (On Diff #430322) | |
llvm/test/DebugInfo/X86/debug-info-template-parameter.ll | ||
33–36 ↗ | (On Diff #430322) | |
mlir/test/Conversion/MemRefToSPIRV/alloc.mlir | ||
44 ↗ | (On Diff #430322) |
llvm/test/CodeGen/AArch64/fp16-v8-instructions.ll | ||
---|---|---|
682–688 ↗ | (On Diff #430322) | These lines should be removed. The were accidentally left in as the file was update_llc_test_check'd |
llvm/test/CodeGen/Thumb2/thumb2-execute-only-prologue.ll | ||
13 ↗ | (On Diff #430322) | I think ; CHECK-NEXT: .long 4294965696 should be OK. That looks like it would match up with the code. |
llvm/test/CodeGen/AArch64/fp16-v8-instructions.ll | ||
---|---|---|
682–688 ↗ | (On Diff #430322) | This was fixed in rG1584b2c74e4c804a2c85d760a1a2c10b33465f2e |
LGTM. Thanks for pinging the relevant folks on individual tests. You can push the fixes yourself before landing this patch.
Rebased on top of fixes for non-trivial cases. Going to submit this
tomorrow, if no objections.
Committing this revealed some more failures, fixed in rG8894c05b0d351f998b1836542ba791247394bd12.
Instead of introducing a new wrapper, why don't you change all the return to call a constructor method (e.g. make_check_type()) that does what this wrapper do? Then there would not be any FindCheckType that take a Misspelled parameter.
I'm also not sure about Misspelled being a check kind. It feels conceptually wrong but on the other hand I guess it makes the implementation simpler.