rjmccall added inline comments.
================
Comment at: include/clang/AST/ASTContext.h:1986
+ /// This object needs to be initialized by Sema the first time it checks
+ /// a three-way comparison.
+ ComparisonCategories CompCategories;
----------------
Is this comment accurate? Because if your test case with a deserialized
comparison operator is supposed to work, we certainly don't get that.
================
Comment at: include/clang/AST/ComparisonCategories.h:71
+ /// standard library. The key is a value of ComparisonCategoryResult.
+ mutable llvm::DenseMap<char, VarDecl *> Objects;
+
----------------
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.)
================
Comment at: include/clang/AST/ComparisonCategories.h:206
+ const ASTContext &Ctx;
+ mutable llvm::DenseMap<char, ComparisonCategoryInfo> Data;
+ mutable NamespaceDecl *StdNS = nullptr;
----------------
Please add a comment explaining what the keys are.
================
Comment at: lib/AST/ComparisonCategories.cpp:85
+ return nullptr;
+}
+
----------------
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.
================
Comment at: lib/CodeGen/CGExprAgg.cpp:971
+ auto EmitCmpRes = [&](const VarDecl *VD) {
+ return CGF.CGM.GetAddrOfGlobalVar(VD);
+ };
----------------
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.
================
Comment at: lib/Sema/SemaDeclCXX.cpp:8929
+ return QualType();
+ }
+
----------------
Alright, works for me.
================
Comment at: lib/Sema/SemaDeclCXX.cpp:8956
+ // the cache and return the newly cached value.
+ FullyCheckedComparisonCategories.insert(Info);
+ return Info->getType();
----------------
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.
================
Comment at: lib/Sema/SemaExpr.cpp:9816
+ RHS = S.ImpCastExprToType(RHS.get(), Type, CK_BitCast);
+ } else {
+ // C++2a [expr.spaceship]p4
----------------
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.
================
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;
----------------
I believe this will assert if the underlying type of the enum is `bool`.
================
Comment at: lib/Sema/SemaExpr.cpp:9955
+ // and direction polls
+ return buildResultTy(ComparisonCategoryType::StrongEquality);
+
----------------
Should we generate a tautological warning about comparisons on `nullptr_t` that
aren't the result of some kind of macro/template expansion?
https://reviews.llvm.org/D45476
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
