Michael137 marked an inline comment as done.
Michael137 added inline comments.
================
Comment at: clang/test/CodeGenObjCXX/encode.mm:93-94
// FIXME: This difference is due to D76801. It was probably an unintentional
change. Maybe we want to undo it?
- // CHECKCXX98: @_ZN11rdar93574002ggE ={{.*}} constant [49 x i8]
c"{vector<float, rdar9357400::fixed<4, -1> >=[4f]}\00"
- // CHECKCXX20: @_ZN11rdar93574002ggE ={{.*}} constant [48 x i8]
c"{vector<float, rdar9357400::fixed<4, -1>>=[4f]}\00"
+ // CHECKCXX98: @_ZN11rdar93574002ggE ={{.*}} constant [45 x i8]
c"{vector<float, rdar9357400::fixed<4> >=[4f]}\00"
+ // CHECKCXX20: @_ZN11rdar93574002ggE ={{.*}} constant [44 x i8]
c"{vector<float, rdar9357400::fixed<4>>=[4f]}\00"
extern const char gg[] = @encode(vector4f);
----------------
aprantl wrote:
> dblaikie wrote:
> > aprantl wrote:
> > > aprantl wrote:
> > > > dblaikie wrote:
> > > > > aprantl wrote:
> > > > > > Michael137 wrote:
> > > > > > > Michael137 wrote:
> > > > > > > > dblaikie wrote:
> > > > > > > > > @aprantl any idea if this is good/OK? (I guess it probably is
> > > > > > > > > - but maybe these strings were never meant to ignore/suppress
> > > > > > > > > default arguments of any kind? or maybe this is an ABI sort
> > > > > > > > > of thing where it suppressing some but not others is now
> > > > > > > > > unchangeable?)
> > > > > > > > Good point. There was a thread on the cfe mailing list a while
> > > > > > > > ago about the last time this broke:
> > > > > > > > https://lists.llvm.org/pipermail/cfe-dev/2020-November/067194.html
> > > > > > > >
> > > > > > > > This was @rsmith's stance:
> > > > > > > > ```
> > > > > > > > I think some of the other recent TypePrinter changes might also
> > > > > > > > risk
> > > > > > > > changing the @encode output. Generally it seems unwise for
> > > > > > > > @encode to be
> > > > > > > > using the type pretty-printer if it wants to be ABI-stable; I
> > > > > > > > don't think
> > > > > > > > it's reasonable to expect any guarantees as to the stability of
> > > > > > > > pretty-printed type names. I think USR generation suffers from
> > > > > > > > similar
> > > > > > > > problems; it too uses the type pretty-printer to generate
> > > > > > > > supposedly-ABI-stable keys in at least some cases.
> > > > > > > > ```
> > > > > > > see https://reviews.llvm.org/D90622
> > > > > > To me it really looks like the intention of the feature is to not
> > > > > > substitute default parameters. But if we stop doing this now it
> > > > > > will likely result in a surprising code size increase, that may not
> > > > > > be considered worth it compared to the risk of breaking ABI by
> > > > > > changing a default template parameter.
> > > > > >
> > > > > > As far as this patch is concerned, it's neutral to this decision
> > > > > > (which may not have been a conscious one).
> > > > > > It's certainly not good that every type printer change is an ABI
> > > > > > break.
> > > > > Awesome, thanks for tracking down that context @Michael137.
> > > > >
> > > > > Not quite sure I'm following you @aprantl, but I think you're saying
> > > > > this change is OK/seems consistent with other changes?
> > > > The current code (for @encode and USRs) seems to assume that
> > > > TypePrinter output is stable.
> > > >
> > > > I (personally) think that assumption ought to be wrong, because
> > > > otherwise we'd never be able to make improvements such as this patch.
> > > >
> > > > Aside from my personal preferences, practically this patch causes a
> > > > problem for shipping an Objective-C compiler, since this is an
> > > > ABI-breaking change. That's why I'd like to hear from someone with more
> > > > insight into how `@encoding` is used in Objective-C wether this is
> > > > something we need to be concerned about. If it is a concern we may need
> > > > to add a TypePrinter configuration optimized for stability that
> > > > preserves the current output format in eternity.
> > > To summarize an offline conversation about this: Because there are no
> > > system frameworks that vend Objective-C++ types we are not concerned by a
> > > potential ABI break caused by this patch.
> > >
> > > LGTM!
> > Awesome - thanks for getting that info, @aprantl - any idea if we could
> > write this down somewhere? (in comments in tests that verify `@encoding` if
> > there aren't too many of them?) So it's easy/clear next time.
> @Michael137 You could add a comment to this very test here, stating that the
> fact that the @encoding for C++ is effectively dependent on the TypePrinter
> implementation is a known bug.
Done
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D139986/new/
https://reviews.llvm.org/D139986
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
