rjmccall added inline comments.
================
Comment at: include/clang/AST/ComparisonCategories.h:71
+ /// standard library. The key is a value of ComparisonCategoryResult.
+ mutable llvm::DenseMap<char, VarDecl *> Objects;
+
----------------
EricWF wrote:
> rjmccall wrote:
> > We expect this map to have at least two of the seven result types, and
> > probably three or four, right? It should probably just be an array; it'll
> > both be faster and use less memory.
> >
> > (The similar map in `ComparisonCategories` is different because we expect
> > it to be empty in most translation units.)
> Ack.
>
> Do you want `std::array` or something slightly more conservative like
> `llvm::SmallVector<T, 4>`?
std::array is definitely better here.
================
Comment at: lib/AST/ComparisonCategories.cpp:85
+ return nullptr;
+}
+
----------------
EricWF wrote:
> rjmccall wrote:
> > This method is returning a pointer to an entry of a DenseMap. The
> > resulting pointer is then treated as a stable key in a set on Sema. That
> > pointer will be dangling if the DenseMap needs to grow beyond its original
> > allocation.
> >
> > I would suggest perhaps storing a fixed-size array of pointers to
> > ComparisonCategoryInfos that you allocate on-demand.
> Woops! Thanks for the correction. I'm so used to STL node-based maps I
> assumed the keys were stable.
>
> I'll use a bitset, and index into it using the `ComparisonCategoryType`
> enumerators as indexes.
Sounds good.
================
Comment at: lib/CodeGen/CGExprAgg.cpp:971
+ auto EmitCmpRes = [&](const VarDecl *VD) {
+ return CGF.CGM.GetAddrOfGlobalVar(VD);
+ };
----------------
EricWF wrote:
> rjmccall wrote:
> > EricWF wrote:
> > > rsmith wrote:
> > > > Perhaps directly emit the constant value here rather than the address
> > > > of the global? I think we should consider what IR we want to see coming
> > > > out of Clang, and I don't think that IR should contain loads from
> > > > globals to get the small constant integer that is the value of the
> > > > conversion result.
> > > >
> > > > I think it would be reasonable for us to say that we require the
> > > > standard library types to contain exactly one non-static data member of
> > > > integral type, and for us to form a select between the relevant integer
> > > > values here. We really have no need to support all possible
> > > > implementations of these types, and we can revisit this if some other
> > > > standard library implementation ships types that don't follow that
> > > > pattern. (If we find such a standard library, we could emit multiple
> > > > selects, or a first-class aggregate select, or whatever generates the
> > > > best code at -O0.)
> > > I agree emitting the value would be better, and that most STL
> > > implementations should implement the types using only one non-static
> > > member.
> > > However, note that the specification for `partial_ordering` is described
> > > in terms of two non-static data members, so it seems possible an STL
> > > implementation might implement in that way.
> > >
> > > Would it be appropriate to do this as a smaller follow up patch?
> > While it would be nice if we could special-case the case of a class with a
> > single integral field all the way through the various uses of it, IRGen is
> > not at all set up to try to take advantage of that. You will need to store
> > your integral result into the dest slot here anyway. That makes me suspect
> > that there's just no value in trying to produce a scalar select before
> > doing that store instead of branching to pick one of two stores.
> >
> > Also, I know there's this whole clever argument for why we can get away
> > with lazily finding this comparison-result type and its static members in
> > translation units that are just deserializing a spaceship operator. Just
> > to make me feel better, though, please actually check here dynamically that
> > the assumptions we're relying on actually hold and that we've found an
> > appropriate variable for the comparison result and it does have an
> > initializer. It is fine to generate an atrocious diagnostic if we see a
> > failure, but let's please not crash just because something weird and
> > unexpected happened with module import.
> > While it would be nice if we could special-case the case of a class with a
> > single integral field all the way through the various uses of it, IRGen is
> > not at all set up to try to take advantage of that. You will need to store
> > your integral result into the dest slot here anyway. That makes me suspect
> > that there's just no value in trying to produce a scalar select before
> > doing that store instead of branching to pick one of two stores.
>
> I went ahead and did it anyway, though I suspect you might be right. I'll
> need to look into it further. (In particular if we avoid ODR uses and hence
> can avoid emitting the inline variable definitions).
>
> > Just to make me feel better, though, please actually check here dynamically
> > that the assumptions we're relying on actually hold and that we've found an
> > appropriate variable for the comparison result and it does have an
> > initializer.
>
> Ack. I've already added checks in `Sema` that validate that the caches have
> been populated correctly, and that the required constraints hold on the
> comparison category type and it's instances.
>
> When we import a module, the Sema checking isn't repeated, but if anything
> from that loaded module requires the comparison category caches, then they
> must have been well-formed when we initially checked them, and so they should
> also be well formed when we lazely populate them.
>
>
>
Right. I do actually believe the clever argument in normal situations. :) It
just seems brittle enough that I don't really trust that there won't be some
corner case that bypasses it, or some future change to modules that breaks it
in ways we fail to anticipate.
================
Comment at: lib/Sema/SemaDeclCXX.cpp:8956
+ // the cache and return the newly cached value.
+ FullyCheckedComparisonCategories.insert(Info);
+ return Info->getType();
----------------
EricWF wrote:
> rjmccall wrote:
> > I think you should probably do this insertion above (perhaps instead of the
> > original `count` check) so that you don't dump 100 diagnostics on the user
> > if they've got a malformed stdlib.
> I don't think that would be correct. For example, the following code should
> only issue one diagnostic.
> ```
> auto foo(int x, int y) { return x <=> y; } // expected-error {{include
> <compare>}}
> #include <compare>
> auto bar(int x, int y) { return x <=> y; } // OK
> ```
>
> Also, like with `<initializer_list>` we probably want the following code to
> emit two diagnostics:
>
> ```
> void foo(int x, int y) {
> (void)(x <=> y); // expected-error
> (void)(x <=> y); // expected-error
> }
> ```
>
> When `<compare>` is ill-formed, I believe the correct behavior is to emit a
> single diagnostic for each expression
> which requires the header. Otherwise, we could end up with a ton of
> ill-formed but undiagnosed code.
>
I think there are ways to handle the subsequent-inclusion thing, but I'm not
too attached to the idea of not emitting redundant errors here.
================
Comment at: lib/Sema/SemaExpr.cpp:9816
+ RHS = S.ImpCastExprToType(RHS.get(), Type, CK_BitCast);
+ } else {
+ // C++2a [expr.spaceship]p4
----------------
EricWF wrote:
> rjmccall wrote:
> > rsmith wrote:
> > > EricWF wrote:
> > > > rsmith wrote:
> > > > > We still need to apply the usual arithmetic conversions after
> > > > > converting enumerations to their underlying types (eg, `<=>` on `enum
> > > > > E : char` converts the operands first to `char` then to `int`). You
> > > > > could remove the `else` here and make this stuff unconditional, but
> > > > > it's probably better to sidestep the extra work and convert directly
> > > > > to the promoted type of the enum's underlying type.
> > > > Do we still do usual arithmetic conversions if we have two enumerations
> > > > of the same type?
> > > Formally, yes: "If both operands have the same enumeration type E, the
> > > operator yields the result of converting the operands to the underlying
> > > type of E and applying <=> to the converted operands."
> > >
> > > The recursive application of `<=>` to the converted operands will perform
> > > the usual arithmetic conversions.
> > `isEnumeralType` is just checking for an any enum type, but I assume we
> > can't use a spaceship to compare a scoped enum type, right? Since the
> > ordinary relational operators aren't allowed either.
> You can compare two enums of the same type (scoped or unscoped), or an
> unscoped enum and an arithmetic type.
>
> (Also, ordinary relational operators are supported for scoped enum types).
Huh. How did I mess that up? Okay, makes sense to me.
================
Comment at: lib/Sema/SemaExpr.cpp:9825
+ LHS = S.ImpCastExprToType(LHS.get(), IntType, CK_IntegralCast);
+ RHS = S.ImpCastExprToType(RHS.get(), IntType, CK_IntegralCast);
+ LHSType = RHSType = IntType;
----------------
EricWF wrote:
> EricWF wrote:
> > rjmccall wrote:
> > > I believe this will assert if the underlying type of the enum is `bool`.
> > Ack. It does indeed.
> >
> > What's the correct way to promote the boolean to an integer?
> I seem to have solved the problem for enums with an underlying type of bool
> by first performing LValueToRValue conversions, followed by a conversion to
> `int` instead of `bool`.
>
> Does that sound reasonable?
A CK_IntegralToBoolean would also be reasonable, I think. Or thinking about,
it also wouldn't be unreasonable to just weaken the assertion when the integral
type is an enum of bool underlying type.
================
Comment at: lib/Sema/SemaExpr.cpp:9955
+ // and direction polls
+ return buildResultTy(ComparisonCategoryType::StrongEquality);
+
----------------
EricWF wrote:
> rjmccall wrote:
> > Should we generate a tautological warning about comparisons on `nullptr_t`
> > that aren't the result of some kind of macro/template expansion?
> Probably? But we don't currently do it for equality operators, so perhaps
> this could be done in a follow up patch which adds the diagnostic for both
> equality and three-way comparisons?
I thought we had some warnings in that space, but maybe not that one. Don't
worry about it.
https://reviews.llvm.org/D45476
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
