- User Since
- May 9 2013, 11:10 AM (297 w, 6 d)
The revision title and summary contradict each other. The title says to allow something, but the summary says that already works; then the summary says CHECK-DAG can't be first, while the check-dag.txt test clearly already has CHECK-DAG as the first directive.
Please fix up the title and summary so that they correctly describe what you are trying to accomplish. This will help reviewers to make sure the patch does what you want.
Fri, Jan 18
Thanks! I'm not surprised I didn't get it completely right; it was such a maze of twisty passages all different.
Mon, Jan 14
LGTM although I do wonder at the usefulness of the wildcards in the implicit-not checks.
Thu, Jan 10
Nice work on the prefix tests. One comment nit and LGTM.
A couple of comment phrasing nits.
Wed, Jan 9
Sorry for not noticing this sooner. The TL;DR is, the current patch does not affect PS4, so go ahead; but see the long version for other considerations.
Tue, Jan 8
A few more comments.
Thanks for the ping. The code change looks straightforward, but a handful of questions on the tests.
Mon, Jan 7
Fri, Jan 4
Thu, Jan 3
Changes to CommandLine.* should be tested in llvm/unittests/Support/CommandLineTest.cpp, as a rule. We don't want to rely on FileCheck tests to verify code in Support.
Dec 20 2018
I am willing to review this; however it is a very large patch, which will take time to go through. Also, after tomorrow, I am on holiday until 3 January.
This will permit -DVALUE= (with no value) is that a supported usage?
It also permits -D=10 which I daresay doesn't mean anything.
Dec 18 2018
A couple small things. If Jonas is happy, I'm happy.
I missed that filcab had said ok internally. Go ahead Pierre.
Dec 17 2018
LGTM but I will defer to @filcab as he is more familiar with the area.
Much better, thanks! LGTM
Dec 14 2018
Note that the lower-bound might be implicit, but it's never "unknown."
We do need to account for the cases where the implicit lower-bound is 1. The Fortran people will need this, and the OpenVMS folks support several other languages that also have a default LB of 1.
The terminology around "possible" "expected" and "final" matches probably warrants a few lines of commentary somewhere, unless I missed it in an earlier patch. There are getting to be enough cases that the casual reader (ahem) can't just Zen their way through them all.
Can be done as a follow-up if you prefer.
One comment nit and LGTM.
Dec 12 2018
LGTM although I think John should have a chance to get in a last word.
These get to be pretty mechanical after a while, don't they? I was wondering how hard it would be for someone else, next time somebody wants to add a new directive kind, but it seems like not really a big deal and that's a good thing.
Again a comment needs fixing but otherwise LGTM.
I'll try to get to the rest tomorrow.
Comment issue, otherwise LGTM.
One small comment fix needed, otherwise LGTM.
A couple of nits I didn't notice until looking at the next patch.
Dec 10 2018
Abandoning dead patch. This wound up being done a different way.
I like having the extra explanations at the end of the tilde lines, that's a great idea.
One unnecessary include left, and LGTM.
Does the verifier bail out after the first failure? If not, you can combine the failure tests all into one test.
Dec 6 2018
Dec 4 2018
Easy enough to accommodate NVPTX, if you use DWARF version to pick constant size versus label diff. I don't remember what it takes to work out what the target is at this point, but version is obviously easy.
Dec 3 2018
You're using !~~~ to flag: (a) CHECK-NOT that has a match, (b) CHECK-NEXT that isn't in the right place, (c) a discarded DAG match.
The first two are errors (which would be reported regardless of annotations) but the third is not. So, I'm not sure why it should have the same kind of annotation.
I have no answer for the address-space questions. I'd say ask on llvm-dev, as it's worth asking in a more visible forum.
Assuming we are able to say that nullptr is zero for address space zero, the patch seems fine (and modifying an existing test is perfectly fine).
It looks like this patch would fail to reject cases such as:
.file 1 "a.c" .file 1 "b.c"
and if that's true, it's a problem.
+ Matt Davis who wrote the test
Sorry for the delay; feel free to add a 'ping' reply every week or so when you get no action.
Seems right to me but I think Matt would be a better choice to review.
The indentation in the test file looks a little funny; please make sure there are no hard tabs in the file. Aside from that LGTM.
Nov 30 2018
LGTM aside from a grump about the test, which is totally up to you.
Apologies for the delay. I haven't been ignoring this series; I was having internal qualms about the amount of effort to produce extensive annotations, and the value they might provide. But I've come down in favor of doing it.
When building the FileCheck binary with debug info, this patch makes the build artifacts ~1kb smaller.
Are those hard tabs in the test .s file? many editors do that automatically in assembler source, but the LLVM project doesn't want hard tabs.
Nov 29 2018
Nov 28 2018
My patches have landed, and look like they'll stick; please rebase and use the DISubprogram specific flags for the new Fortran flags.
Nov 27 2018
I am curious how debug info for common blocks plays with LTO. I think we would not want different common-block descriptions to be de-duplicated just because the global name was the same.
Don't emit unused fields to the bitcode. Accommodate the new variant in the reader.
Nov 26 2018
Nov 21 2018
This is small enough that I'd prefer to see it incorporated into the patch with the "real" warning, rather than have this temporary placeholder diagnostic.