Bugfix#349 #361
Bugfix#349 #361
Conversation
| @@ -1,7 +1,5 @@ | |||
| // RUN: 3c -alltypes %s | FileCheck -match-full-lines -check-prefixes="CHECK_ALL","CHECK" %s | |||
There was a problem hiding this comment.
Was this test not passing at all, before? I see you have not changed the expectation of what -alltypes will produce, in terms of changing the source file. So I'm confused how you are testing the change that you made to ConstraintVariables.cpp.
There was a problem hiding this comment.
This was one of the tests that I previously identified for not running clang on the output of 3c. It turns out, that would cause an error, explained in more detail with simplified example in issue #349. This change was verified by the addition of the compilation line, and the lack of effect on the suite of regression tests. I was hesitant to change the test in other ways, since I didn't write it or analyze why it exists. But I can add a "CHECKALL" line at the location that allowed it to compile.
|
The solution here is to base the typedef level of the new constraint variable on the computed type, rather than the input type. The computed type is used to create the atoms, so this makes sense. The generic type is represented internally as a typedef, so relying on that code for proper rewriting works. There's still a matter of over-constraint, shown in the xfail test, but that can be handled later with more thorough inference of generics. |
Thanks for the explanation. It would be useful to make a comment for this just above the new code location; i.e., "compute the typedef level here so it's based on the computed type, not the input type" |
| // Issue #349. Check that the parameter doesn't inherit the double pointer argument within do_doubleptr | ||
| _Itype_for_any(T) | ||
| void incoming_doubleptr(void* ptr : itype(_Array_ptr<T>)) { | ||
| // CHECK_ALL: void incoming_doubleptr(_Array_ptr<T> ptr : itype(_Array_ptr<T>)) { |
There was a problem hiding this comment.
Again, this seems very weird to me. I'm not sure why the void * component is changing. As far as I am aware, we are assuming that ITypes will not change during conversion. Perhaps we are slowly moving away from that. @jackastner maybe you can comment?
There was a problem hiding this comment.
I does seem odd that this ends up with both _Array_ptr<T> and itype(_Array_ptr<T>)). For the most part, any itypes in the source code should be copied unmodified to the output.
I think the fix this PR makes means that the type of ptr is _Array_ptr<T> rather than _Array_ptr<_Ptr<T>>. This is definitely the better rewriting, but we might want to avoid rewriting the type altogether.
There was a problem hiding this comment.
Yes, before the fix it was being rewritten with two pointer levels. But it is safe, so shouldn't it be rewritten with the new type and no Itype? Or is that just how it will be done with the liberal itypes enhancement?
There was a problem hiding this comment.
Two things: (1) It doesn't make sense to have a checked pointer to the left of the colon, on an itype. Having T: itype(T) ought to just be T. (2) We have not been, by default, rewriting pointers in itypes that appear in the source. I think we should stick with this for now. We can consider what changes make sense in a broader issue/PR later, that is disentangled from the issue here (wrong instantiations, and multiple levels of abstraction).
There was a problem hiding this comment.
Perhaps I misunderstood the bug. In regards to (2), this bugfix does not add the rewriting, it only makes it correct. It was being rewritten in the test originally, but there was no CHECK line to verify this. I can look into the rewrite code to try to prevent the rewriting. I assume... anytime PointerVariableConstraint::ItypeStr is non-empty?
There was a problem hiding this comment.
I see. It shouldn't have been getting rewritten, I don't believe. So maybe that's a bug in addition? Are these bugs something one can tease apart?
There was a problem hiding this comment.
If you'll accept separate bugfixes for the two issues mentioned above, than this one as-is fixes the correctness issue, and is ready to merge pending any conflicts with code merged in the mean time. I'll make a new issue for the duplicated type, which is inconvenient but compiles correctly.
There was a problem hiding this comment.
OK. Just to get it on the record: By "duplicated type" you mean that void * is being rewritten to _Array_ptr<T> which duplicates the type in : itype(_Array_ptr<T>) on the same line. The root type (here, void *) should not get rewritten, and that's a separate issue we can tackle (#367).
|
|
||
| void do_doubleptr(int count) { | ||
| int **arr = malloc(sizeof(int*) * count); | ||
| // CHECK: _Array_ptr<_Ptr<int>> arr : count(count) = malloc<_Ptr<int>>(sizeof(int*) * count); |
There was a problem hiding this comment.
Just to be clear: This is the rewriting that should happen, but does not? What happens instead?
There was a problem hiding this comment.
It is rewritten as _Array_ptr<_Array_ptr<int>> arr, because of interference from the other function.
I was mistaken in #349, using the typedef code to handle this aspect of generics does solve the compile error in
hash.c