Soften error#414 #432
Soften error#414 #432
Conversation
mattmccutchen-cci
dismissed
their stale review
My main concerns have been addressed, but I'm not prepared to approve the whole PR.
|
Thanks for updating the tests and fixing my obvious mistakes in the RUN lines (which I see now). I would still prefer to see multi-file declaration + definition cases, but if others don't feel it is important, I won't insist on it. |
| PVConstraint *From = dyn_cast<PVConstraint>(FromCV); | ||
| assert(From != nullptr); | ||
| CAtoms CFrom = From->getCvars(); | ||
| assert(Vars.size() == CFrom.size()); | ||
| if(Vars.size() != CFrom.size()) { | ||
| ReasonFailed = "transplanting between pointers with different depths"; |
There was a problem hiding this comment.
Is it ever possible for ReasonFailed to be non-empty when it gets here? In mergeDeclaration you are checking if it was already set, since there are recursive calls.
There was a problem hiding this comment.
(I think not, and you've probably checked; just wondering.)
There was a problem hiding this comment.
I did miss the recursive call. Partly because I have no idea what FV is. What is FV? I'll give it a modified reason if necessary with a return.
There was a problem hiding this comment.
added a function pointer fail case and an internal merge fail case, both with returns.
clang/lib/3C/ProgramInfo.cpp
Outdated
| // assume a global function, but change to a static if not | ||
| ExternalFunctionMapType *Map = &ExternalFunctionFVCons; | ||
| if (!FD->isGlobal()) { | ||
| // if the filename is new, just insert and we're done |
There was a problem hiding this comment.
What does it mean for the filename to be "new" ?
There was a problem hiding this comment.
Not yet seen in the map. I'll improve the wording.
There was a problem hiding this comment.
I like the refactoring of the insertion code for FV constraints! I don't understand @mattmccutchen-cci 's comment about multi-file tests -- aren't there multi-file tests in this PR (involving function foo) ?
| // By transplanting the atoms of OldC into NewC, we ensure that any | ||
| // constraints applied to NewC later on constrain the atoms of OldC. | ||
| NewC->brainTransplant(OldC, *this); | ||
| void ProgramInfo::insertNewFVConstraint(FunctionDecl *FD, FVConstraint *NewC, |
There was a problem hiding this comment.
The old functions were removed from ProgramInfo.h; why was this new function not added there?
There was a problem hiding this comment.
It always existed as the entry point and branched into the two functions now removed. (But since 2/3 of then were minor code then call then next, I combined them all)
We have That leaves us to consider tests of the conflicting declaration + definition case that is being fixed in this PR. This PR adds My sense was that the multi-file case is slightly more likely to break than the single-file case, so I would have preferred that the new tests use multiple files. But (as I believe Kyle was arguing) the chance of a change breaking the multi-file declaration+definition case without breaking either the multi-file two-declaration case or the single-file declaration+definition case seems low, so this may not be worth doing now, especially given that the code may be reworked again before too long (e.g., as part of #433). |
|
I see. Then my question is: Is the reason for the absence of the test you are asking for because it won't actually pass, and you are proposing that we should add it and also make further changes so that it does? I see the link to #433, which is a long discussion I don't have time to read ATM. But if I get the gist of it and what you are saying, there are larger problems afoot that we should not leave for too long, and perhaps the missing/unsupported case you are talking about can be considered with them. Perhaps copy your above comment to #433 too, so we don't drop it once this PR is merged? |
No, the code in this PR is fine and the test would pass. For concreteness, I've gone ahead and written it: here's a change that adds the case I was talking about ( |
|
OK sounds good. Let's get this merged, and we will welcome additional tests later of the form you propose. Thanks! |
|
This PR is now integrated into a #463, a more encompassing change which removes half the code here anyway. |
Continuation of #421 with correctcomputation branch. I improved the code based on the comments and added a couple tests. I also ran the nightly tests. They mostly failed, but the same ones passed/failed as in the latest scheduled run, so I think that means it's good?