Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
558193003:
Redesign of the internal type system. (Closed)
DescriptionRedesign of the internal type system.
Besides addressing a fundamental flaw, this significantly simplifies
several aspects of the system. The downside is a loss of precision
and a loss of algebraic properties.
Range types are now fully implemented.
R=rossberg@chromium.org
BUG=
Committed: https://code.google.com/p/v8/source/detail?r=24163
Patch Set 1 #
Total comments: 37
Patch Set 2 : #Patch Set 3 : This addresses the issue I pointed out on Friday. In order to keep the changes as simple as possib… #
Total comments: 42
Patch Set 4 : #Patch Set 5 : #
Total comments: 6
Patch Set 6 : #Patch Set 7 : What I told you about yesterday. #Patch Set 8 : Fixing compile error on Windows and a bug in Max() #Patch Set 9 : Fix another Windows build error. #Patch Set 10 : Dito #
Total comments: 16
Messages
Total messages: 11 (0 generated)
A first batch of comments; need more time for the union & intersect stuff... https://codereview.chromium.org/558193003/diff/1/src/types.cc File src/types.cc (left): https://codereview.chromium.org/558193003/diff/1/src/types.cc#oldcode83 src/types.cc:83: int TypeImpl<Config>::BitsetType::InherentLub(TypeImpl* type) { I'm glad that we can get rid of this one... https://codereview.chromium.org/558193003/diff/1/src/types.cc File src/types.cc (right): https://codereview.chromium.org/558193003/diff/1/src/types.cc#newcode98 src/types.cc:98: if (type->IsBitset()) return type->AsBitset(); Put this case first, since it is probably the most common one. https://codereview.chromium.org/558193003/diff/1/src/types.cc#newcode266 src/types.cc:266: return kMaxUInt32; Why not static_cast<double>(kMaxUint32) + 1, and avoid the <= vs < pitfall below? https://codereview.chromium.org/558193003/diff/1/src/types.cc#newcode275 src/types.cc:275: int TypeImpl<Config>::BitsetType::Lub(Limits lim) { Nit: move this decl closer to the other numeric Lubs https://codereview.chromium.org/558193003/diff/1/src/types.cc#newcode281 src/types.cc:281: if (min < Min(kOtherSigned32)) { Hm, can't this written more concisely using a loop over a little table? https://codereview.chromium.org/558193003/diff/1/src/types.cc#newcode301 src/types.cc:301: if (min < Min(kOtherNumber)) { Shouldn't this be <= ? https://codereview.chromium.org/558193003/diff/1/src/types.cc#newcode314 src/types.cc:314: bool TypeImpl<Config>::SimplyEquals(TypeImpl* that) { Is this not supposed to handle bitsets? If so, add an assertion against this (and Union, I suppose). https://codereview.chromium.org/558193003/diff/1/src/types.cc#newcode375 src/types.cc:375: if (that->IsBitset()) { Always handle the bitset case first where possible -- it should be fastest. https://codereview.chromium.org/558193003/diff/1/src/types.cc#newcode435 src/types.cc:435: // (iff T is not a union) I'm confused... https://codereview.chromium.org/558193003/diff/1/src/types.h File src/types.h (right): https://codereview.chromium.org/558193003/diff/1/src/types.h#newcode222 src/types.h:222: * Otherwise: Nit: indentation off https://codereview.chromium.org/558193003/diff/1/src/types.h#newcode678 src/types.h:678: StructuralType::New(StructuralType::kClassTag, 1, region)); Nit: style guide wants +4 indentation for line continuations. https://codereview.chromium.org/558193003/diff/1/src/types.h#newcode698 src/types.h:698: return this->template GetValue<i::Object>(0); Nit: probably fits on previous line https://codereview.chromium.org/558193003/diff/1/src/types.h#newcode717 src/types.h:717: // They represent continuous integer intervals in the form of a minimum This should go to the top, along with the other explanations of the type system. https://codereview.chromium.org/558193003/diff/1/src/types.h#newcode729 src/types.h:729: double Max() { return MaxV()->Number(); } Since we call the method to get a numeric value from an object 'Value' in other places, I think it would be more consistent to name these 4 methods Min, Max, MinValue, MaxValue. https://codereview.chromium.org/558193003/diff/1/src/types.h#newcode738 src/types.h:738: #ifdef DEBUG Debugging left-overs? https://codereview.chromium.org/558193003/diff/1/test/cctest/test-types.cc File test/cctest/test-types.cc (right): https://codereview.chromium.org/558193003/diff/1/test/cctest/test-types.cc#ne... test/cctest/test-types.cc:278: if (min->Number() > max->Number()) std::swap(min, max); Oh you nasty imperative programmer... https://codereview.chromium.org/558193003/diff/1/test/cctest/test-types.cc#ne... test/cctest/test-types.cc:453: // Intersect(T1, T2) is almost bitwise conjunction for bitsets T1,T2 Can't harm to be more precise. :) "is ... upto uninhabited results" or something
https://codereview.chromium.org/558193003/diff/1/src/types.cc File src/types.cc (left): https://codereview.chromium.org/558193003/diff/1/src/types.cc#oldcode83 src/types.cc:83: int TypeImpl<Config>::BitsetType::InherentLub(TypeImpl* type) { On 2014/09/10 15:44:14, rossberg wrote: > I'm glad that we can get rid of this one... Well, it's basically still there. But we only have one notion of Lub now. https://codereview.chromium.org/558193003/diff/1/src/types.cc File src/types.cc (right): https://codereview.chromium.org/558193003/diff/1/src/types.cc#newcode98 src/types.cc:98: if (type->IsBitset()) return type->AsBitset(); On 2014/09/10 15:44:13, rossberg wrote: > Put this case first, since it is probably the most common one. Done. https://codereview.chromium.org/558193003/diff/1/src/types.cc#newcode266 src/types.cc:266: return kMaxUInt32; On 2014/09/10 15:44:14, rossberg wrote: > Why not static_cast<double>(kMaxUint32) + 1, and avoid the <= vs < pitfall > below? Right, that's what it should have been in the first place. https://codereview.chromium.org/558193003/diff/1/src/types.cc#newcode275 src/types.cc:275: int TypeImpl<Config>::BitsetType::Lub(Limits lim) { On 2014/09/10 15:44:14, rossberg wrote: > Nit: move this decl closer to the other numeric Lubs I moved the Lub for maps up. The order now matches the order in the main Lub function. https://codereview.chromium.org/558193003/diff/1/src/types.cc#newcode281 src/types.cc:281: if (min < Min(kOtherSigned32)) { On 2014/09/10 15:44:13, rossberg wrote: > Hm, can't this written more concisely using a loop over a little table? I rewrote it and also got rid of the Min() function. https://codereview.chromium.org/558193003/diff/1/src/types.cc#newcode301 src/types.cc:301: if (min < Min(kOtherNumber)) { On 2014/09/10 15:44:14, rossberg wrote: > Shouldn't this be <= ? The <= below should have been < but due to the bug above it was wrong anyway. https://codereview.chromium.org/558193003/diff/1/src/types.cc#newcode314 src/types.cc:314: bool TypeImpl<Config>::SimplyEquals(TypeImpl* that) { On 2014/09/10 15:44:14, rossberg wrote: > Is this not supposed to handle bitsets? If so, add an assertion against this > (and Union, I suppose). Done. https://codereview.chromium.org/558193003/diff/1/src/types.cc#newcode375 src/types.cc:375: if (that->IsBitset()) { On 2014/09/10 15:44:14, rossberg wrote: > Always handle the bitset case first where possible -- it should be fastest. Ok. I rewrote this slightly. https://codereview.chromium.org/558193003/diff/1/src/types.cc#newcode435 src/types.cc:435: // (iff T is not a union) On 2014/09/10 15:44:14, rossberg wrote: > I'm confused... Hehe. Line 435 is a leftover from older code. https://codereview.chromium.org/558193003/diff/1/src/types.h File src/types.h (right): https://codereview.chromium.org/558193003/diff/1/src/types.h#newcode222 src/types.h:222: * Otherwise: On 2014/09/10 15:44:15, rossberg wrote: > Nit: indentation off Done. https://codereview.chromium.org/558193003/diff/1/src/types.h#newcode678 src/types.h:678: StructuralType::New(StructuralType::kClassTag, 1, region)); On 2014/09/10 15:44:15, rossberg wrote: > Nit: style guide wants +4 indentation for line continuations. Done. https://codereview.chromium.org/558193003/diff/1/src/types.h#newcode698 src/types.h:698: return this->template GetValue<i::Object>(0); On 2014/09/10 15:44:15, rossberg wrote: > Nit: probably fits on previous line Done. https://codereview.chromium.org/558193003/diff/1/src/types.h#newcode717 src/types.h:717: // They represent continuous integer intervals in the form of a minimum On 2014/09/10 15:44:15, rossberg wrote: > This should go to the top, along with the other explanations of the type system. Done. https://codereview.chromium.org/558193003/diff/1/src/types.h#newcode729 src/types.h:729: double Max() { return MaxV()->Number(); } On 2014/09/10 15:44:15, rossberg wrote: > Since we call the method to get a numeric value from an object 'Value' in other > places, I think it would be more consistent to name these 4 methods Min, Max, > MinValue, MaxValue. Not sure I'm following. Are you suggesting to expand the V to Value (fine with me), or to rename Max to MaxValue and MaxV to Max (not so fine with me). https://codereview.chromium.org/558193003/diff/1/src/types.h#newcode738 src/types.h:738: #ifdef DEBUG On 2014/09/10 15:44:15, rossberg wrote: > Debugging left-overs? Done. https://codereview.chromium.org/558193003/diff/1/test/cctest/test-types.cc File test/cctest/test-types.cc (right): https://codereview.chromium.org/558193003/diff/1/test/cctest/test-types.cc#ne... test/cctest/test-types.cc:453: // Intersect(T1, T2) is almost bitwise conjunction for bitsets T1,T2 On 2014/09/10 15:44:15, rossberg wrote: > Can't harm to be more precise. :) "is ... upto uninhabited results" or something Acknowledged.
Second batch... https://codereview.chromium.org/558193003/diff/1/src/types.h File src/types.h (right): https://codereview.chromium.org/558193003/diff/1/src/types.h#newcode729 src/types.h:729: double Max() { return MaxV()->Number(); } On 2014/09/11 12:58:12, neis1 wrote: > On 2014/09/10 15:44:15, rossberg wrote: > > Since we call the method to get a numeric value from an object 'Value' in > other > > places, I think it would be more consistent to name these 4 methods Min, Max, > > MinValue, MaxValue. > > Not sure I'm following. Are you suggesting to expand the V to Value (fine with > me), or to rename Max to MaxValue and MaxV to Max (not so fine with me). The latter. It's more consistent with our naming elsewhere. https://codereview.chromium.org/558193003/diff/40001/src/types.cc File src/types.cc (right): https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode20 src/types.cc:20: DisallowHeapAllocation no_allocation; Can't this function be simplified to return Limit(std::max(lhs.min, rhs.min), std::min(lhs.max, rhs.max)) ? And likewise union. Maybe add a comment here that this intentionally allows for min>max. https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode80 src/types.cc:80: return unioned->Get(0)->BitsetGlb(); // Shortcut. I don't think this comment got clearer ;) https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode258 src/types.cc:258: int atoms31[] = { Can we zip these arrays into two tables over a struct{ bitset type; double min; }? That would make it more readable. https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode361 src/types.cc:361: if (this->IsRange()) { Do the that->IsRange() test first, to avoid the duplication. https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode424 src/types.cc:424: if (!BitsetType::IsInhabited(this->BitsetLub() & that->BitsetLub())) I'm not sure I understand why this is the right thing, given the new asymmetric definition of IsInhabited. https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode429 src/types.cc:429: if (that->IsClass() && !this->IsClass()) return true; You can combine these two into "if (this->IsClass() != that->IsClass()) ..." https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode480 src/types.cc:480: // 3. At most one element is a range, and it must be the second one. Can't you have a range and no bitset? https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode559 src/types.cc:559: if (range->Is(result->Get(1))) return size; Maybe bind to a variable Get(1), instead of repeating it several times. Also, I think you only want to compare ranges here, right? Instead of going through the (more expensive) general Is, maybe test IsRange and then do a Contains on the ranges directly. Similarly below. https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode560 src/types.cc:560: if (!result->Get(1)->Is(range->unhandle())) { Is the unhandle needed? https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode568 src/types.cc:568: if (result->Get(i)->Is(range->unhandle())) { Likewise, here you only need to consider constants. https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode703 src/types.cc:703: // Add [this] to [result] unless [this] is bitset, range, or already subsumed. s/this/type/g https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode729 src/types.cc:729: if (size >= 2 && unioned->Get(1)->Is(unioned->Get(0))) { This condition looks weird. Can't slot 0 contain whatever at this point? https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode983 src/types.cc:983: os << "Function["; Hm? https://codereview.chromium.org/558193003/diff/40001/src/types.h File src/types.h (right): https://codereview.chromium.org/558193003/diff/40001/src/types.h#newcode527 src/types.h:527: static Limits intersect(Limits lhs, Limits rhs); Nit: use uppercase names. https://codereview.chromium.org/558193003/diff/40001/src/types.h#newcode574 src/types.h:574: return bitset & kSemantic; Hm, well, this doesn't really reflect the function name anymore. https://codereview.chromium.org/558193003/diff/40001/test/cctest/test-types.cc File test/cctest/test-types.cc (right): https://codereview.chromium.org/558193003/diff/40001/test/cctest/test-types.c... test/cctest/test-types.cc:460: // Intersect(T1, T2) is bitwise conjunction for bitsets T1,T2 (modulo None) Hm, None doesn't really sound right...
https://codereview.chromium.org/558193003/diff/1/src/types.h File src/types.h (right): https://codereview.chromium.org/558193003/diff/1/src/types.h#newcode729 src/types.h:729: double Max() { return MaxV()->Number(); } On 2014/09/15 15:58:29, rossberg wrote: > On 2014/09/11 12:58:12, neis1 wrote: > > On 2014/09/10 15:44:15, rossberg wrote: > > > Since we call the method to get a numeric value from an object 'Value' in > > other > > > places, I think it would be more consistent to name these 4 methods Min, > Max, > > > MinValue, MaxValue. > > > > Not sure I'm following. Are you suggesting to expand the V to Value (fine > with > > me), or to rename Max to MaxValue and MaxV to Max (not so fine with me). > > The latter. It's more consistent with our naming elsewhere. But less consistent with the naming here, such as StructuralType::GetValue, StructuralType::SetValue, Constant::Value, where Value means an Ojbect pointer. https://codereview.chromium.org/558193003/diff/40001/src/types.cc File src/types.cc (right): https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode20 src/types.cc:20: DisallowHeapAllocation no_allocation; On 2014/09/15 15:58:29, rossberg wrote: > Can't this function be simplified to > > return Limit(std::max(lhs.min, rhs.min), std::min(lhs.max, rhs.max)) > > ? And likewise union. No, that would compare the objects instead of the contained numbers. > Maybe add a comment here that this intentionally allows for min>max. Done. https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode80 src/types.cc:80: return unioned->Get(0)->BitsetGlb(); // Shortcut. On 2014/09/15 15:58:29, rossberg wrote: > I don't think this comment got clearer ;) Done. https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode258 src/types.cc:258: int atoms31[] = { On 2014/09/15 15:58:29, rossberg wrote: > Can we zip these arrays into two tables over a struct{ bitset type; double min; > }? That would make it more readable. Yes, something like that. I also need to pull the data out again, because I need some functionality for the typer that relies on that data. I will also try to rewrite Lub(double) to make use of it. https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode361 src/types.cc:361: if (this->IsRange()) { On 2014/09/15 15:58:29, rossberg wrote: > Do the that->IsRange() test first, to avoid the duplication. Not sure what exactly you had in mind, but I rewrote it. https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode424 src/types.cc:424: if (!BitsetType::IsInhabited(this->BitsetLub() & that->BitsetLub())) On 2014/09/15 15:58:29, rossberg wrote: > I'm not sure I understand why this is the right thing, given the new asymmetric > definition of IsInhabited. I allow the representation part to be empty, but not the semantic part. I think that in order to allow types with empty semantic part as well, we would need to allow non-trivial subtyping between bitsets and structural types again. Since I don't currently see a use for such types, I did it this way, requiring only a tiny change or two. https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode429 src/types.cc:429: if (that->IsClass() && !this->IsClass()) return true; On 2014/09/15 15:58:29, rossberg wrote: > You can combine these two into "if (this->IsClass() != that->IsClass()) ..." Done. https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode480 src/types.cc:480: // 3. At most one element is a range, and it must be the second one. On 2014/09/15 15:58:29, rossberg wrote: > Can't you have a range and no bitset? Yes, but in that case you either have another type (which can then take the bitset's place) or you don't need a union. https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode559 src/types.cc:559: if (range->Is(result->Get(1))) return size; On 2014/09/15 15:58:29, rossberg wrote: > Maybe bind to a variable Get(1), instead of repeating it several times. Done. > Also, I think you only want to compare ranges here, right? Instead of going > through the (more expensive) general Is, maybe test IsRange and then do a > Contains on the ranges directly. Similarly below. Oh, I would really like to avoid that. How about we leave it as-is but keep it in mind in case of performance issues? https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode560 src/types.cc:560: if (!result->Get(1)->Is(range->unhandle())) { On 2014/09/15 15:58:30, rossberg wrote: > Is the unhandle needed? Yes, doesn't compile otherwise. https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode568 src/types.cc:568: if (result->Get(i)->Is(range->unhandle())) { On 2014/09/15 15:58:30, rossberg wrote: > Likewise, here you only need to consider constants. See above. https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode703 src/types.cc:703: // Add [this] to [result] unless [this] is bitset, range, or already subsumed. On 2014/09/15 15:58:29, rossberg wrote: > s/this/type/g Done. https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode729 src/types.cc:729: if (size >= 2 && unioned->Get(1)->Is(unioned->Get(0))) { On 2014/09/15 15:58:29, rossberg wrote: > This condition looks weird. Can't slot 0 contain whatever at this point? Yes. The code is correct but it's a little twisted and I just rewrote it. https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode983 src/types.cc:983: os << "Function["; On 2014/09/15 15:58:29, rossberg wrote: > Hm? I found it impossible to parse non-trivial function types, in particular if an argument or result type is a union. Adding this delimiter made it a little easier. https://codereview.chromium.org/558193003/diff/40001/src/types.h File src/types.h (right): https://codereview.chromium.org/558193003/diff/40001/src/types.h#newcode527 src/types.h:527: static Limits intersect(Limits lhs, Limits rhs); On 2014/09/15 15:58:30, rossberg wrote: > Nit: use uppercase names. Done. https://codereview.chromium.org/558193003/diff/40001/src/types.h#newcode574 src/types.h:574: return bitset & kSemantic; On 2014/09/15 15:58:30, rossberg wrote: > Hm, well, this doesn't really reflect the function name anymore. Depends on your definition of inhabited ;) I have no good idea for a different name. IsSemanticallyInhabited? IsPopulated? HasSemantics? https://codereview.chromium.org/558193003/diff/40001/test/cctest/test-types.cc File test/cctest/test-types.cc (right): https://codereview.chromium.org/558193003/diff/40001/test/cctest/test-types.c... test/cctest/test-types.cc:460: // Intersect(T1, T2) is bitwise conjunction for bitsets T1,T2 (modulo None) On 2014/09/15 15:58:30, rossberg wrote: > Hm, None doesn't really sound right... Why not? If Intersect(T1, T2) is None, then it may not equal the bitwise conjunction of T1 and T2. Also, this fits into one line :) If that doesn't convince you, I can rewrite it to "Intersect(T1, T2) is bitwise conjunction for bitsets T1,T2 (unless the latter is uninhabited, in which case the intersection is None)."
> https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode258 > src/types.cc:258: int atoms31[] = { > On 2014/09/15 15:58:29, rossberg wrote: > > Can we zip these arrays into two tables over a struct{ bitset type; double > min; > > }? That would make it more readable. > > Yes, something like that. I also need to pull the data out again, because I > need some functionality for the typer that relies on that data. I will also try > to rewrite Lub(double) to make use of it. The last patch does this. The added Min() and Max() functions are what I want to use in the typer.
https://codereview.chromium.org/558193003/diff/1/src/types.h File src/types.h (right): https://codereview.chromium.org/558193003/diff/1/src/types.h#newcode729 src/types.h:729: double Max() { return MaxV()->Number(); } On 2014/09/16 10:03:59, neis1 wrote: > On 2014/09/15 15:58:29, rossberg wrote: > > On 2014/09/11 12:58:12, neis1 wrote: > > > On 2014/09/10 15:44:15, rossberg wrote: > > > > Since we call the method to get a numeric value from an object 'Value' in > > > other > > > > places, I think it would be more consistent to name these 4 methods Min, > > Max, > > > > MinValue, MaxValue. > > > > > > Not sure I'm following. Are you suggesting to expand the V to Value (fine > > with > > > me), or to rename Max to MaxValue and MaxV to Max (not so fine with me). > > > > The latter. It's more consistent with our naming elsewhere. > > But less consistent with the naming here, such as StructuralType::GetValue, > StructuralType::SetValue, Constant::Value, where Value means an Ojbect pointer. OK, fair enough. Actually, I'd say, just remove the 'double' wrappers from the interface, and rename the other two to Min and Max. https://codereview.chromium.org/558193003/diff/40001/src/types.cc File src/types.cc (right): https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode361 src/types.cc:361: if (this->IsRange()) { On 2014/09/16 10:03:59, neis1 wrote: > On 2014/09/15 15:58:29, rossberg wrote: > > Do the that->IsRange() test first, to avoid the duplication. > > Not sure what exactly you had in mind, but I rewrote it. That's what I had in mind. :) https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode424 src/types.cc:424: if (!BitsetType::IsInhabited(this->BitsetLub() & that->BitsetLub())) On 2014/09/16 10:04:00, neis1 wrote: > On 2014/09/15 15:58:29, rossberg wrote: > > I'm not sure I understand why this is the right thing, given the new > asymmetric > > definition of IsInhabited. > > I allow the representation part to be empty, but not the semantic part. I think > that in order to allow types with empty semantic part as well, we would need to > allow non-trivial subtyping between bitsets and structural types again. Since I > don't currently see a use for such types, I did it this way, requiring only a > tiny change or two. Hm. The asymmetry is too suspicious for my taste. I don't see how it can be justified semantically. https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode480 src/types.cc:480: // 3. At most one element is a range, and it must be the second one. On 2014/09/16 10:03:59, neis1 wrote: > On 2014/09/15 15:58:29, rossberg wrote: > > Can't you have a range and no bitset? > > Yes, but in that case you either have another type (which can then take the > bitset's place) or you don't need a union. Right, figured that out later. Maybe say so explicitly, e.g. add "(even if first one is not a bitset)". https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode559 src/types.cc:559: if (range->Is(result->Get(1))) return size; On 2014/09/16 10:04:00, neis1 wrote: > On 2014/09/15 15:58:29, rossberg wrote: > > Also, I think you only want to compare ranges here, right? Instead of going > > through the (more expensive) general Is, maybe test IsRange and then do a > > Contains on the ranges directly. Similarly below. > > Oh, I would really like to avoid that. How about we leave it as-is but keep it > in mind in case of performance issues? I think it would actually be cleaner conceptually, since this is a function on RangeTypes specifically. Moreover, it's kind of fishy that you check against old_range without even checking that it is a RangeType. Is that actually an invariant here? Shouldn't there be an assertion then, or an AsRange? https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode560 src/types.cc:560: if (!result->Get(1)->Is(range->unhandle())) { On 2014/09/16 10:04:00, neis1 wrote: > On 2014/09/15 15:58:30, rossberg wrote: > > Is the unhandle needed? > > Yes, doesn't compile otherwise. That is weird. What's the error? (I'd really like to get rid of unhandle altogether, it's an ugly hack.) https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode983 src/types.cc:983: os << "Function["; On 2014/09/16 10:03:59, neis1 wrote: > On 2014/09/15 15:58:29, rossberg wrote: > > Hm? > > I found it impossible to parse non-trivial function types, in particular if an > argument or result type is a union. Adding this delimiter made it a little > easier. I see, but this syntax is a bit too random (and potentially, long); it should at least use consistent parens. https://codereview.chromium.org/558193003/diff/80001/src/types.cc File src/types.cc (right): https://codereview.chromium.org/558193003/diff/80001/src/types.cc#newcode675 src/types.cc:675: if (lhs->IsBitset()) { Hm, I wonder if this code wouldn't become clearer if you moved the IsRange tests out. https://codereview.chromium.org/558193003/diff/80001/src/types.h File src/types.h (right): https://codereview.chromium.org/558193003/diff/80001/src/types.h#newcode448 src/types.h:448: double Min(); These really only make sense on number types. So they should at least return a Maybe<double>. https://codereview.chromium.org/558193003/diff/80001/src/types.h#newcode586 src/types.h:586: struct Numeral { Can't type and tables be moved to the private section as well?
I also noticed that the Min/Max functions were not quite correct and fixed this. https://codereview.chromium.org/558193003/diff/1/src/types.h File src/types.h (right): https://codereview.chromium.org/558193003/diff/1/src/types.h#newcode729 src/types.h:729: double Max() { return MaxV()->Number(); } On 2014/09/16 14:04:40, rossberg wrote: > On 2014/09/16 10:03:59, neis1 wrote: > > On 2014/09/15 15:58:29, rossberg wrote: > > > On 2014/09/11 12:58:12, neis1 wrote: > > > > On 2014/09/10 15:44:15, rossberg wrote: > > > > > Since we call the method to get a numeric value from an object 'Value' > in > > > > other > > > > > places, I think it would be more consistent to name these 4 methods Min, > > > Max, > > > > > MinValue, MaxValue. > > > > > > > > Not sure I'm following. Are you suggesting to expand the V to Value (fine > > > with > > > > me), or to rename Max to MaxValue and MaxV to Max (not so fine with me). > > > > > > The latter. It's more consistent with our naming elsewhere. > > > > But less consistent with the naming here, such as StructuralType::GetValue, > > StructuralType::SetValue, Constant::Value, where Value means an Ojbect > pointer. > > OK, fair enough. Actually, I'd say, just remove the 'double' wrappers from the > interface, and rename the other two to Min and Max. Done. https://codereview.chromium.org/558193003/diff/40001/src/types.cc File src/types.cc (right): https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode424 src/types.cc:424: if (!BitsetType::IsInhabited(this->BitsetLub() & that->BitsetLub())) On 2014/09/16 14:04:40, rossberg wrote: > On 2014/09/16 10:04:00, neis1 wrote: > > On 2014/09/15 15:58:29, rossberg wrote: > > > I'm not sure I understand why this is the right thing, given the new > > asymmetric > > > definition of IsInhabited. > > > > I allow the representation part to be empty, but not the semantic part. I > think > > that in order to allow types with empty semantic part as well, we would need > to > > allow non-trivial subtyping between bitsets and structural types again. Since > I > > don't currently see a use for such types, I did it this way, requiring only a > > tiny change or two. > > Hm. The asymmetry is too suspicious for my taste. I don't see how it can be > justified semantically. Marked as TODO. https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode480 src/types.cc:480: // 3. At most one element is a range, and it must be the second one. On 2014/09/16 14:04:40, rossberg wrote: > On 2014/09/16 10:03:59, neis1 wrote: > > On 2014/09/15 15:58:29, rossberg wrote: > > > Can't you have a range and no bitset? > > > > Yes, but in that case you either have another type (which can then take the > > bitset's place) or you don't need a union. > > Right, figured that out later. Maybe say so explicitly, e.g. add "(even if first > one is not a bitset)". Done. https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode559 src/types.cc:559: if (range->Is(result->Get(1))) return size; On 2014/09/16 14:04:40, rossberg wrote: > On 2014/09/16 10:04:00, neis1 wrote: > > On 2014/09/15 15:58:29, rossberg wrote: > > > Also, I think you only want to compare ranges here, right? Instead of going > > > through the (more expensive) general Is, maybe test IsRange and then do a > > > Contains on the ranges directly. Similarly below. > > > > Oh, I would really like to avoid that. How about we leave it as-is but keep > it > > in mind in case of performance issues? > > I think it would actually be cleaner conceptually, since this is a function on > RangeTypes specifically. Moreover, it's kind of fishy that you check against > old_range without even checking that it is a RangeType. Is that actually an > invariant here? Shouldn't there be an assertion then, or an AsRange? As discussed, it might be None too. I documented that with an assertion now. https://codereview.chromium.org/558193003/diff/40001/src/types.cc#newcode983 src/types.cc:983: os << "Function["; On 2014/09/16 14:04:40, rossberg wrote: > On 2014/09/16 10:03:59, neis1 wrote: > > On 2014/09/15 15:58:29, rossberg wrote: > > > Hm? > > > > I found it impossible to parse non-trivial function types, in particular if an > > argument or result type is a union. Adding this delimiter made it a little > > easier. > > I see, but this syntax is a bit too random (and potentially, long); it should at > least use consistent parens. I got rid of it. https://codereview.chromium.org/558193003/diff/80001/src/types.cc File src/types.cc (right): https://codereview.chromium.org/558193003/diff/80001/src/types.cc#newcode675 src/types.cc:675: if (lhs->IsBitset()) { On 2014/09/16 14:04:40, rossberg wrote: > Hm, I wonder if this code wouldn't become clearer if you moved the IsRange tests > out. Done. https://codereview.chromium.org/558193003/diff/80001/src/types.h File src/types.h (right): https://codereview.chromium.org/558193003/diff/80001/src/types.h#newcode448 src/types.h:448: double Min(); On 2014/09/16 14:04:40, rossberg wrote: > These really only make sense on number types. So they should at least return a > Maybe<double>. As discussed, I kept it double but added an assertion. https://codereview.chromium.org/558193003/diff/80001/src/types.h#newcode586 src/types.h:586: struct Numeral { On 2014/09/16 14:04:40, rossberg wrote: > Can't type and tables be moved to the private section as well? Done.
LGTM, with a few comments https://codereview.chromium.org/558193003/diff/170001/src/types.cc File src/types.cc (right): https://codereview.chromium.org/558193003/diff/170001/src/types.cc#newcode332 src/types.cc:332: if (min < BitsetMins()[i].min) { Nit: make a local for BitsetMins(). Similarly below. https://codereview.chromium.org/558193003/diff/170001/src/types.h File src/types.h (right): https://codereview.chromium.org/558193003/diff/170001/src/types.h#newcode711 src/types.h:711: int BitsetLub() { This can't go here like this, as bitsets are an implementation detail. You can return it as a proper Type though, perhaps naming the method Bound or something. https://codereview.chromium.org/558193003/diff/170001/src/types.h#newcode745 src/types.h:745: int BitsetLub() { return this->Get(0)->AsBitset(); } Same here. https://codereview.chromium.org/558193003/diff/170001/test/cctest/test-types.cc File test/cctest/test-types.cc (left): https://codereview.chromium.org/558193003/diff/170001/test/cctest/test-types.... test/cctest/test-types.cc:1093: // Of(V)->Is(T) implies T->Contains(V) Why is this gone? https://codereview.chromium.org/558193003/diff/170001/test/cctest/test-types.... test/cctest/test-types.cc:1134: // NowOf(V)->NowIs(T) implies T->NowContains(V) And this? https://codereview.chromium.org/558193003/diff/170001/test/cctest/test-types.... test/cctest/test-types.cc:1225: // Constant(V)->Maybe(Class(M)) never And these? https://codereview.chromium.org/558193003/diff/170001/test/cctest/test-types.cc File test/cctest/test-types.cc (right): https://codereview.chromium.org/558193003/diff/170001/test/cctest/test-types.... test/cctest/test-types.cc:593: // Functionality & Injectivity: Range(min1, max1) = Range(min2, max2) <=> Nit: Line break after ":" https://codereview.chromium.org/558193003/diff/170001/test/cctest/test-types.... test/cctest/test-types.cc:1414: // This does NOT hold! Can you add a counter example for each property that no longer holds?
https://codereview.chromium.org/558193003/diff/170001/src/types.cc File src/types.cc (right): https://codereview.chromium.org/558193003/diff/170001/src/types.cc#newcode332 src/types.cc:332: if (min < BitsetMins()[i].min) { On 2014/09/23 15:20:37, rossberg wrote: > Nit: make a local for BitsetMins(). Similarly below. Done. https://codereview.chromium.org/558193003/diff/170001/src/types.h File src/types.h (right): https://codereview.chromium.org/558193003/diff/170001/src/types.h#newcode711 src/types.h:711: int BitsetLub() { On 2014/09/23 15:20:38, rossberg wrote: > This can't go here like this, as bitsets are an implementation detail. You can > return it as a proper Type though, perhaps naming the method Bound or something. Oops, forgot about that. I wanted to avoid the "little hack". Done. https://codereview.chromium.org/558193003/diff/170001/src/types.h#newcode745 src/types.h:745: int BitsetLub() { return this->Get(0)->AsBitset(); } On 2014/09/23 15:20:38, rossberg wrote: > Same here. Done. https://codereview.chromium.org/558193003/diff/170001/test/cctest/test-types.cc File test/cctest/test-types.cc (left): https://codereview.chromium.org/558193003/diff/170001/test/cctest/test-types.... test/cctest/test-types.cc:1093: // Of(V)->Is(T) implies T->Contains(V) On 2014/09/23 15:20:38, rossberg wrote: > Why is this gone? We already test it (indirectly): // If Of(V)->Is(T), then Constant(V)->Is(T) // T->Contains(V) iff Constant(V)->Is(T) https://codereview.chromium.org/558193003/diff/170001/test/cctest/test-types.... test/cctest/test-types.cc:1134: // NowOf(V)->NowIs(T) implies T->NowContains(V) On 2014/09/23 15:20:38, rossberg wrote: > And this? Same here. https://codereview.chromium.org/558193003/diff/170001/test/cctest/test-types.... test/cctest/test-types.cc:1225: // Constant(V)->Maybe(Class(M)) never On 2014/09/23 15:20:38, rossberg wrote: > And these? They simply don't hold anymore. I can keep them in there, commented out. But it's odd that exactly these two were stated, and no other combinations, because these two should have never been true in the first place, whereas many of the remaining combinations should. https://codereview.chromium.org/558193003/diff/170001/test/cctest/test-types.cc File test/cctest/test-types.cc (right): https://codereview.chromium.org/558193003/diff/170001/test/cctest/test-types.... test/cctest/test-types.cc:593: // Functionality & Injectivity: Range(min1, max1) = Range(min2, max2) <=> On 2014/09/23 15:20:38, rossberg wrote: > Nit: Line break after ":" Done. https://codereview.chromium.org/558193003/diff/170001/test/cctest/test-types.... test/cctest/test-types.cc:1414: // This does NOT hold! On 2014/09/23 15:20:38, rossberg wrote: > Can you add a counter example for each property that no longer holds? Okay, I will do that in one of the next CLs. I have already one prepared that adds more tests.
Message was sent while issue was closed.
Committed patchset #10 manually as 24163. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
