Soften error

[kyleheadley]

Fix for issue #414. I added a failue reason to braintransplant and changed some of those asserts into diagnostic failures.

I also refactored insertNewFVConstraint for clarity.

I need to add a couple tests, including failure cases, but people expressed interest in this PR as soon as the code was ready.

[john-h-kastner]

The reorganization of insertNewFVConstraint looks nice. A definite improvement over the old code.

[mattmccutchen-cci]

I need to add a couple tests, including failure cases

While I was working on #412, I noticed that the existing tests of the declaration merging errors ({difftypes,multidef}_xfail{1,2}.c) were written as XFAILs, but that's not right: exiting with an error is the expected behavior for the 3c tool (even if we change the exact code path in the future per #283), and removing the XFAIL will make it feasible to test that the error is what we expect and not an unrelated 3C failure. I drafted a commit to fix this and was going to submit it as a separate PR, but I realized it probably makes more sense if you include the change in this PR and follow the same pattern for the new tests you add. You can just cherry-pick my commit into this PR (although it is based on #412, so you'll get some merge conflicts unless you wait for #412 to get merged and update this PR to be based on #412 first).

[kyleheadley]

@mattmccutchen-cci Yeah, I'll update these. Thanks for the test refs, I thought XFAIL meant expected fail? There's also -verify which allows diagnostic checks inline with Filecheck rather than the grep commands you used. I'm looking at how to do that properly now (you may have seen expect-no-diagnostics, which is part of that functionality). So far, googling has come up with lots of complex examples and no simple explanation. I'm continuing soon after a break.

[mattmccutchen-cci]

I thought XFAIL meant expected fail?

XFAIL means that the test is expected to fail because it asserts that the program has the intended behavior, but the actual behavior does not match the intended behavior due to a known bug. Our scenario here is different: we are testing 3C's error reporting functionality, so a 3C failure (nonzero exit) is actually the intended behavior. People may be confused if we use XFAIL in this scenario.

Also, XFAIL has a major practical limitation that it will "pass" if any step fails, so there isn't a good way to test that the "failure" is precisely the declaration merging error message you expect and not some unrelated problem. (In principle, the same argument applies to a test of a known bug: ideally, we should check that the actual behavior reflects the known bug and not some other problem. But lit currently doesn't have a good way to do this, and I don't think it's worth worrying about now.)

There's also -verify which allows diagnostic checks inline with Filecheck rather than the grep commands you used. I'm looking at how to do that properly now (you may have seen expect-no-diagnostics, which is part of that functionality). So far, googling has come up with lots of complex examples and no simple explanation. I'm continuing soon after a break.

Here is the reference on Clang's built-in diagnostic verifier (which I linked to from 3CStandalone.cpp). While the diagnostic verifier uses similar comments as FileCheck, it does not actually use FileCheck; they are separate tools.

That said, I don't think we can use the diagnostic verifier in this scenario because it normally exits 0 or 1 depending on whether the diagnostics were as expected, but here you are exiting 1 before the verifier has a chance to verify anything. We might be able to add more code to allow the verifier to work; I don't think it's worthwhile now, though it may be worth considering in the future if we add more immediate-exit errors to 3C. Given all that, if you have a better idea than the grep, be my guest!

[kyleheadley]

Since there's a testing refactor (#412) nearly complete, I'll defer until that merges to add the test files. Then we can use the code suggested a few comments up and create a similar one for the error reported here.

[kyleheadley]

Switched to #432 with a new branch on correctcomputation.