- User Since
- May 9 2013, 11:10 AM (314 w, 5 d)
The helper is passed as a template parameter to a class ctor, and that instantiated class calls the helper from operator(), so I suppose maybe enough indirection through lambdas....
but this seems kind of involved for getting my builds to work.
To provide some missing details:
The original source gets a bogus compile-time error with Visual Studio 2017, prior to version 15.8. I have 15.2, so I need this patch to build Clang.
Our current minimum-version requirement (per CheckCompilerVersion.cmake) is Visual Studio 2015 (some specific update), which assumes all Visual Studio 2017 versions are usable. So by that criterion, we need this patch for all Visual Studio 2017 versions to be usable.
Arguably the anonymous-namespace version is the "more modern" tactic anyway, so as a long-term fix this is actually fine anyway.
Fri, May 17
That's a great idea, especially the "string variable" and "string substitution". I was not pleased with the name either but the only thing I thought of was regex variable which is plain wrong, I completely missed the obvious.
Naming is hard, I clearly didn't think of it right away either.
Thu, May 16
There's still some inconsistency in the terminology, as clearly evidenced by the name and description of the IsNumExpr member of FileCheckPatternSubstitution. That class used to be where we'd find variables (now called pattern variables); but it has had numeric expressions crammed into it, except the comment wants to call it a substitution, and so we have "pattern" and "substitution" and "expression" all competing for clarity.
Wed, May 15
Minor stuff. This solution is surprisingly simple, the main question being (and I don't have an answer) whether increasing the size of PresumedLoc is okay.
It still bothers me that MCDwarf has to know exactly when to override the target's decision to do relaxation.
Tue, May 14
It sounds like inlining a function with a loop has a bug with respect to how the loop metadata is handled, and your patch merely tripped over that. Would it be reasonable to fix the inlining-loop-metadata bug separately first? And then the original patch is likely to Just Work?
Fixing one bug at a time is more in line with project practices.
LGTM once you rephrase the comment in the test.
LGTM after you add the full-stop noted below.
Mon, May 13
Starting set of comments on this one. Haven't looked at everything yet.
Fri, May 10
Thu, May 9
@stella.stamenova fixed this using system-windows instead.
@stella.stamenova I'm not familiar with any lit feature that gives a special meaning to the prefix "no". The opposite of "REQUIRES: windows" is not "REQUIRES: nowindows" but "UNSUPPORTED: windows" AFAIK.
This part of the discussion should probably be taken to llvm-dev, though.
I am pretty happy with this now, maybe give the other people who have been participating a day or so to chime in again.
Wed, May 8
One or two last nits and LGTM.
I wonder if this should be an MIR test? Fewer moving parts.
Sorry, I should have been more thorough about this previously: My understanding is that if a function description mentions any parameters with \p, then it needs to mention all of them.
For some of the one-liner descriptions, if you prefer to just remove the \p that's okay with me. You've been suffering a lot through this process and I don't want to create more work than we really need to!
Fix the one typo and LGTM.
The point is not that a certain set of moves comes out, but that the *same* set of moves comes out regardless of debug info.
So, the test wants to have two copies of the same function, one with debug instructions and one without; and both functions should produce the same sequence of moves.
Aha. The new versions add the word "pattern" to some of the diagnostics, in pattern-defines-diagnostics.txt. That would be the functional change belonging to D60385, which means that test should not pass with an unmodified FileCheck? Please verify.
Tue, May 7
I still can't tell what the functional difference is, in pattern-defines.txt. Everything else looks fine.
Thanks for the reference, that helps some.
However I still think this isn't the right way to go about fixing the problem. This started because RISC-V decided not to resolve certain kinds of expressions, and then it turns out that applies to too many expressions. Somebody suggested that the asm backend decision should be more refined, based on symbol section perhaps, and that feels like a more principled way to go about it.
+1 to Bjorn's comment.
The DWARF spec says address class is also allowed on a "subroutine or subroutine type" do we allow generating code into nonzero address spaces?
Mon, May 6
Nearly there! One final grammar nit and some test comments.
I don't claim to be an MCExpr expert, but I think this should not be necessary. There are other situations where symbol differences are computed and emitted as constants, for example when DW_AT_high_pc is a length rather than an address, and they clearly don't require this tactic.
Fri, May 3
Checked-in files should not have CRLF endings, in general (there are a very few exceptions, and these aren't among them).
If the checked-in files have LF endings but your tool checks them out with CRLF then that is a problem with your config, or with the tool.
Adding dos2unix to the RUN lines is the wrong fix, either way.
Thu, May 2
Thanks! Some small things, and LGTM once you've fixed those.
Wed, May 1
Haven't looked at the tests yet.
Another round of commentary nits, plus a couple more substantive remarks. If you accept them all, LGTM.
FYI, we had to disable clangd entirely after this patch, getting a weird problem with lit that we haven't figured out yet.
In case anybody else has seen something like this, and knows what's going on, please let us know.
Running all regression tests llvm-lit.py: C:/j/w/opensource/opensource_to_sce_merge__2/o/llvm\utils\lit\lit\llvm\config.py:336: fatal: couldn't find 'clang' program, try setting CLANG in your environment C:\Program Files (x86)\MSBuild\Microsoft.Cpp\v4.0\V140\Microsoft.CppCommon.targets(171,5): error MSB6006: "cmd.exe" exited with code 2. [C:\j\w\opensource\opensource_to_sce_merge__2\build\check-all.vcxproj]
Tue, Apr 30
First pass looking at docs and commentary.
Fri, Apr 26
I had tried to do this in r333311 but some bots complained, so I reverted it and then didn't follow through. Thanks for doing this!
When I tried it, I took advantage of existing tracking of line directives at the file level, so there shouldn't be a need to add a Line flag to PresumedLoc?
What I did was something like this, in computeChecksum():
const SrcMgr::SLocEntry &Entry = SM.getSLocEntry(FID, &Invalid); if (Invalid || !Entry.isFile() || Entry.getFile().hasLineDirectives()) return None;
Thu, Apr 25
I'm okay with this, but give @aprantl a chance to confirm.
I'm okay with the PS4-specific bits.
Wed, Apr 24
LGTM after the comment on the parseVariable loop is addressed.
Tue, Apr 23
LGTM. The previous diagnostic would be useful only to compiler developers.
Apr 18 2019
Apr 16 2019
I guess I'm not clear what your final goal is for the option. Keep it, even though GCC doesn't have one like it? Eliminate it? Please clearly state what you intend to have in the end, and what you might have in the short term in case that is different.